Re: [edk2-devel] [PATCH 08/10] OvmfPkg: suppress "Value stored to ... is never read" analyzer warnings

["Jordan Justen" <jordan.l.justen@intel.com>]

On 2019-04-12 16:31:26, Laszlo Ersek wrote:
> RH covscan warns about assignments that it can determine are never
> "consumed" later ("dead stores"). The idea behind the warning is
> presumably that the programmer forgot to implement a dependent check
> somewhere.
> 
> For each such warning that has been emitted for OvmfPkg,
> the case is one of the following however:
> 
> - We assign a variable a value for (re-)initialization's sake, in
>   preparation for further or future uses. This practice is safe (sometimes
>   even recommended!), hence we should suppress these warnings.
> 
> - We capture a result or a computation in a variable, following a general
>   programming pattern, but then decide to ignore the value in that
>   particular case. This is again safe, and we should suppress these
>   warnings too.
> 
> According to the Clang documentation at
> 
>   https://clang-analyzer.llvm.org/faq.html#dead_store
> 
> we should use
> 
>   (void)x;
> 
> See the logs below (produced originally for edk2-stable201903).
> 
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:156:3: warning: Value
> > stored to 'NumberOfTableEntries' is never read
> > #  NumberOfTableEntries = 0;
> > #  ^                      ~
> > edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:156:3: note: Value
> > stored to 'NumberOfTableEntries' is never read
> > #  NumberOfTableEntries = 0;
> > #  ^                      ~
> > #  154|     DsdtTable   = NULL;
> > #  155|     TableHandle = 0;
> > #  156|->   NumberOfTableEntries = 0;
> > #  157|
> > #  158|     //
> >
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c:43:3:
> > warning: Value stored to 'SetupSize' is never read
> > #  SetupSize = 0;
> > #  ^           ~
> > edk2-89910a39dcfd/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c:43:3:
> > note: Value stored to 'SetupSize' is never read
> > #  SetupSize = 0;
> > #  ^           ~
> > #   41|
> > #   42|     SetupBuf = NULL;
> > #   43|->   SetupSize = 0;
> > #   44|     KernelBuf = NULL;
> > #   45|     KernelInitialSize = 0;
> >
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.c:609:9:
> > warning: Value stored to 'VariableDataBufferSize' is never read
> > #        VariableDataBufferSize = 0;
> > #        ^                        ~
> > edk2-89910a39dcfd/OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.c:609:9:
> > note: Value stored to 'VariableDataBufferSize' is never read
> > #        VariableDataBufferSize = 0;
> > #        ^                        ~
> > #  607|           FreePool (VariableData);
> > #  608|           VariableData = NULL;
> > #  609|->         VariableDataBufferSize = 0;
> > #  610|         }
> > #  611|         VariableData = AllocatePool (VariableDataSize);
> >
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/Library/VirtioLib/VirtioLib.c:125:3: warning:
> > Value stored to 'RingPagesPtr' is never read
> > #  RingPagesPtr += sizeof *Ring->Used.AvailEvent;
> > #  ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > edk2-89910a39dcfd/OvmfPkg/Library/VirtioLib/VirtioLib.c:125:3: note:
> > Value stored to 'RingPagesPtr' is never read
> > #  RingPagesPtr += sizeof *Ring->Used.AvailEvent;
> > #  ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > #  123|
> > #  124|     Ring->Used.AvailEvent = (volatile VOID *) RingPagesPtr;
> > #  125|->   RingPagesPtr += sizeof *Ring->Used.AvailEvent;
> > #  126|
> > #  127|     Ring->QueueSize = QueueSize;
> >
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c:1079:3:
> > warning: Value stored to 'FwhInstance' is never read
> > #  FwhInstance = (EFI_FW_VOL_INSTANCE *)
> > #  ^             ~~~~~~~~~~~~~~~~~~~~~~~
> > edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c:1079:3:
> > note: Value stored to 'FwhInstance' is never read
> > #  FwhInstance = (EFI_FW_VOL_INSTANCE *)
> > #  ^             ~~~~~~~~~~~~~~~~~~~~~~~
> > # 1077|     ASSERT_RETURN_ERROR (PcdStatus);
> > # 1078|
> > # 1079|->   FwhInstance = (EFI_FW_VOL_INSTANCE *)
> > # 1080|       (
> > # 1081|         (UINTN) ((UINT8 *) FwhInstance) + FwVolHeader->HeaderLength +
> >
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c:173:3:
> > warning: Value stored to 'Status' is never read
> > #  Status = gDS->RemoveMemorySpace (
> > #  ^        ~~~~~~~~~~~~~~~~~~~~~~~~
> > edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c:173:3:
> > note: Value stored to 'Status' is never read
> > #  Status = gDS->RemoveMemorySpace (
> > #  ^        ~~~~~~~~~~~~~~~~~~~~~~~~
> > #  171|     // Mark flash region as runtime memory
> > #  172|     //
> > #  173|->   Status = gDS->RemoveMemorySpace (
> > #  174|                     BaseAddress,
> > #  175|                     Length
> >
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/SataControllerDxe/SataController.c:231:3:
> > warning: Value stored to 'DeviceUDmaMode' is never read
> > #  DeviceUDmaMode = 0;
> > #  ^                ~
> > edk2-89910a39dcfd/OvmfPkg/SataControllerDxe/SataController.c:231:3:
> > note: Value stored to 'DeviceUDmaMode' is never read
> > #  DeviceUDmaMode = 0;
> > #  ^                ~
> > #  229|     UINT16    DeviceUDmaMode;
> > #  230|
> > #  231|->   DeviceUDmaMode = 0;
> > #  232|
> > #  233|     //
> >
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/VirtioGpuDxe/Gop.c:65:5: warning: Value stored
> > to 'Status' is never read
> > #    Status = VirtioGpuSetScanout (
> > #    ^        ~~~~~~~~~~~~~~~~~~~~~
> > edk2-89910a39dcfd/OvmfPkg/VirtioGpuDxe/Gop.c:65:5: note: Value stored to
> > 'Status' is never read
> > #    Status = VirtioGpuSetScanout (
> > #    ^        ~~~~~~~~~~~~~~~~~~~~~
> > #   63|       // by setting ResourceId=0 for it.
> > #   64|       //
> > #   65|->     Status = VirtioGpuSetScanout (
> > #   66|                  VgpuGop->ParentBus, // VgpuDev
> > #   67|                  0, 0, 0, 0,         // X, Y, Width, Height
> >
> > Error: CLANG_WARNING:
> > edk2-89910a39dcfd/OvmfPkg/VirtioNetDxe/SnpReceive.c:165:3: warning:
> > Value stored to 'RxPtr' is never read
> > #  RxPtr += sizeof (UINT16);
> > #  ^        ~~~~~~~~~~~~~~~
> > edk2-89910a39dcfd/OvmfPkg/VirtioNetDxe/SnpReceive.c:165:3: note: Value
> > stored to 'RxPtr' is never read
> > #  RxPtr += sizeof (UINT16);
> > #  ^        ~~~~~~~~~~~~~~~
> > #  163|       *Protocol = (UINT16) ((RxPtr[0] << 8) | RxPtr[1]);
> > #  164|     }
> > #  165|->   RxPtr += sizeof (UINT16);
> > #  166|
> > #  167|     Status = EFI_SUCCESS;
> 
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710
> Issue: scan-0992.txt
> Issue: scan-0996.txt
> Issue: scan-0997.txt
> Issue: scan-0998.txt
> Issue: scan-1000.txt
> Issue: scan-1001.txt
> Issue: scan-1006.txt
> Issue: scan-1011.txt
> Issue: scan-1012.txt
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/Xen.c                                 | 5 +++++
>  OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c           | 5 +++++
>  OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.c | 4 ++++
>  OvmfPkg/Library/VirtioLib/VirtioLib.c                         | 5 +++++
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c       | 5 +++++
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c    | 5 +++++
>  OvmfPkg/SataControllerDxe/SataController.c                    | 5 +++++
>  OvmfPkg/VirtioGpuDxe/Gop.c                                    | 6 ++++++
>  OvmfPkg/VirtioNetDxe/SnpReceive.c                             | 5 +++++
>  9 files changed, 45 insertions(+)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/Xen.c b/OvmfPkg/AcpiPlatformDxe/Xen.c
> index 357c60d23f4e..c8f275a8ee84 100644
> --- a/OvmfPkg/AcpiPlatformDxe/Xen.c
> +++ b/OvmfPkg/AcpiPlatformDxe/Xen.c
> @@ -144,16 +144,21 @@ InstallXenTables (
>    Fadt2Table  = NULL;
>    Fadt1Table  = NULL;
>    Facs2Table  = NULL;
>    Facs1Table  = NULL;
>    DsdtTable   = NULL;
>    TableHandle = 0;
>    NumberOfTableEntries = 0;
>
> +  //
> +  // Suppress "Value stored to ... is never read" analyzer warnings.
> +  //
> +  (VOID)NumberOfTableEntries;
> +

I've seen this solution to shut up the compiler for similar reasons,
and it kind of bugs me.

It looks like both paths of the if & else initialize
NumberOfTableEntries, so maybe we can just drop setting it to 0?

I looked at FwhInstance too, and it doesn't look like it is used after
being set. So, maybe that should just be dropped?

I guess both of your bullet points give arguments allowing the unused
set.

Rather than adding "Suppress..." comments everywhere, maybe a global
macro could be defined for similar uses in EDK II?

//
// Suppress "Value stored to ... is never read" analyzer warnings.
//
#define IGNORE_UNUSED_ASSIGNMENT(var) ((VOID)var)

Then again, I guess we decided to suppress this in EDK II compiler
warnings with -Wno-unused-but-set-variable.

So, can you adjust the RH covscan tool settings to similarly ignore
these warnings?

-Jordan

>    //
>    // Try to find Xen ACPI tables
>    //
>    Status = GetXenAcpiRsdp (&XenAcpiRsdpStructurePtr);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
> index ddfef925edd3..fde8c40218a4 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
> @@ -37,16 +37,21 @@ TryRunningQemuKernel (
>    SetupSize = 0;
>    KernelBuf = NULL;
>    KernelInitialSize = 0;
>    CommandLine = NULL;
>    CommandLineSize = 0;
>    InitrdData = NULL;
>    InitrdSize = 0;
>
> +  //
> +  // Suppress "Value stored to ... is never read" analyzer warnings.
> +  //
> +  (VOID)SetupSize;
> +
>    if (!QemuFwCfgIsAvailable ()) {
>      return EFI_NOT_FOUND;
>    }
>
>    QemuFwCfgSelectItem (QemuFwCfgItemKernelSize);
>    KernelSize = (UINTN) QemuFwCfgRead64 ();
>
>    QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
> diff --git a/OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.c b/OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.c
> index a6cebcb54f6b..64fe9d9be543 100644
> --- a/OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.c
> +++ b/OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.c
> @@ -596,16 +596,20 @@ SerializeVariablesIterateSystemVariables (
>        //
>        // The currently allocated VariableData buffer is too small,
>        // so we allocate a larger buffer.
>        //
>        if (VariableDataBufferSize != 0) {
>          FreePool (VariableData);
>          VariableData = NULL;
>          VariableDataBufferSize = 0;
> +        //
> +        // Suppress "Value stored to ... is never read" analyzer warnings.
> +        //
> +        (VOID)VariableDataBufferSize;
>        }
>        VariableData = AllocatePool (VariableDataSize);
>        if (VariableData == NULL) {
>          Status = EFI_OUT_OF_RESOURCES;
>          break;
>        }
>        VariableDataBufferSize = VariableDataSize;
>
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 555d2a5ce7c9..0e4b8b6b265e 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -113,16 +113,21 @@ VirtioRingInit (
>    RingPagesPtr += sizeof *Ring->Used.Idx;
>
>    Ring->Used.UsedElem = (volatile VOID *) RingPagesPtr;
>    RingPagesPtr += sizeof *Ring->Used.UsedElem * QueueSize;
>
>    Ring->Used.AvailEvent = (volatile VOID *) RingPagesPtr;
>    RingPagesPtr += sizeof *Ring->Used.AvailEvent;
>
> +  //
> +  // Suppress "Value stored to ... is never read" analyzer warnings.
> +  //
> +  (VOID)RingPagesPtr;
> +
>    Ring->QueueSize = QueueSize;
>    return EFI_SUCCESS;
>  }
>
>
>  /**
>
>    Tear down the internal resources of a configured virtio ring.
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> index edf438a422fa..ff6565865846 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -1071,16 +1071,21 @@ FvbInitialize (
>    ASSERT_RETURN_ERROR (PcdStatus);
>
>    FwhInstance = (EFI_FW_VOL_INSTANCE *)
>      (
>        (UINTN) ((UINT8 *) FwhInstance) + FwVolHeader->HeaderLength +
>        (sizeof (EFI_FW_VOL_INSTANCE) - sizeof (EFI_FIRMWARE_VOLUME_HEADER))
>      );
>
> +  //
> +  // Suppress "Value stored to ... is never read" analyzer warnings.
> +  //
> +  (VOID)FwhInstance;
> +
>    //
>    // Module type specific hook.
>    //
>    InstallVirtualAddressChangeHandler ();
>
>    PcdStatus = PcdSetBoolS (PcdOvmfFlashVariablesEnable, TRUE);
>    ASSERT_RETURN_ERROR (PcdStatus);
>    return EFI_SUCCESS;
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> index 69b20916bc7c..979122f2e2fd 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> @@ -164,16 +164,21 @@ MarkIoMemoryRangeForRuntimeAccess (
>    //
>    // Mark flash region as runtime memory
>    //
>    Status = gDS->RemoveMemorySpace (
>                    BaseAddress,
>                    Length
>                    );
>
> +  //
> +  // Suppress "Value stored to ... is never read" analyzer warnings.
> +  //
> +  (VOID)Status;
> +
>    Status = gDS->AddMemorySpace (
>                    EfiGcdMemoryTypeMemoryMappedIo,
>                    BaseAddress,
>                    Length,
>                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>                    );
>    ASSERT_EFI_ERROR (Status);
>
> diff --git a/OvmfPkg/SataControllerDxe/SataController.c b/OvmfPkg/SataControllerDxe/SataController.c
> index 8d6a6bbb2286..0cf6b42c886a 100644
> --- a/OvmfPkg/SataControllerDxe/SataController.c
> +++ b/OvmfPkg/SataControllerDxe/SataController.c
> @@ -219,16 +219,21 @@ CalculateBestUdmaMode (
>    OUT UINT16            *SelectedMode
>    )
>  {
>    UINT16    TempMode;
>    UINT16    DeviceUDmaMode;
>
>    DeviceUDmaMode = 0;
>
> +  //
> +  // Suppress "Value stored to ... is never read" analyzer warnings.
> +  //
> +  (VOID)DeviceUDmaMode;
> +
>    //
>    // Check whether the WORD 88 (supported UltraDMA by drive) is valid
>    //
>    if ((IdentifyData->AtaData.field_validity & 0x04) == 0x00) {
>      return EFI_UNSUPPORTED;
>    }
>
>    DeviceUDmaMode = IdentifyData->AtaData.ultra_dma_mode;
> diff --git a/OvmfPkg/VirtioGpuDxe/Gop.c b/OvmfPkg/VirtioGpuDxe/Gop.c
> index 0b2659d1d2b9..d0f81c349f73 100644
> --- a/OvmfPkg/VirtioGpuDxe/Gop.c
> +++ b/OvmfPkg/VirtioGpuDxe/Gop.c
> @@ -57,16 +57,22 @@ ReleaseGopResources (
>      // by setting ResourceId=0 for it.
>      //
>      Status = VirtioGpuSetScanout (
>                 VgpuGop->ParentBus, // VgpuDev
>                 0, 0, 0, 0,         // X, Y, Width, Height
>                 0,                  // ScanoutId
>                 0                   // ResourceId
>                 );
> +
> +    //
> +    // Suppress "Value stored to ... is never read" analyzer warnings.
> +    //
> +    (VOID)Status;
> +
>      //
>      // HACK BEGINS HERE
>      //
>      // According to the GPU Device section of the VirtIo specification, the
>      // above operation is valid:
>      //
>      // "The driver can use resource_id = 0 to disable a scanout."
>      //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpReceive.c b/OvmfPkg/VirtioNetDxe/SnpReceive.c
> index cdee9a2aee47..51b2f8a4e370 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpReceive.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpReceive.c
> @@ -153,16 +153,21 @@ VirtioNetReceive (
>    }
>    RxPtr += SIZE_OF_VNET (Mac);
>
>    if (Protocol != NULL) {
>      *Protocol = (UINT16) ((RxPtr[0] << 8) | RxPtr[1]);
>    }
>    RxPtr += sizeof (UINT16);
>
> +  //
> +  // Suppress "Value stored to ... is never read" analyzer warnings.
> +  //
> +  (VOID)RxPtr;
> +
>    Status = EFI_SUCCESS;
>
>  RecycleDesc:
>    ++Dev->RxLastUsed;
>
>    //
>    // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
>    //
> -- 
> 2.19.1.3.g30247aa5d201
> 
> 
> 
> 
>