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-15 09:25:33, Laszlo Ersek wrote:
> On 04/14/19 10:03, Jordan Justen wrote:
> > 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?
> 
> It might trigger *other* compilers to whine about "variable read without
> initialization" :)
> 
> > 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?
> 
> I think that should be possible, yes. AIUI it is supposed to have
> location-sensitive permanent overrides (maintained outside of the source
> code), and it should be doing incremental / differential scanning --
> report only "new" issues.

I think if the setting can be tweaked, maybe most of these changes
wouldn't be needed. But, it also might prevent you from having to
"fix" similar issues in the future.

> Admittedly, I hate most of the warning suppression patches myself,
> littering our code -- and that applies to all the *historical* spurious
> assignments that we already have in place --, so I'm 100% fine with
> dropping this patch (and other suppression patches too, if that is the
> upstream edk2 project's preference!)

In MdePkg/Include/X64/ProcessorBind.h, I see:

//
// Disable ICC's remark #593: "Variable" was set but never used.
// This is legal ANSI C code so we disable the remark that is turned on with /W4
//
#pragma warning ( disable : 593 )

And, then we have -Wno-unused-but-set-variable for GCC.

Regarding "spurious assignments", MdePkg/Include/X64/ProcessorBind.h,
I also see:

//
// This warning is for potentially uninitialized local variable, and it may cause false
// positive issues in VS2013 and VS2015 build
//
#pragma warning ( disable : 4701 )

-Jordan