From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: jordan.l.justen@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Sun, 14 Apr 2019 01:03:45 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Apr 2019 01:03:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,348,1549958400"; d="scan'208";a="337195376" Received: from jgoodma2-mobl.amr.corp.intel.com (HELO localhost) ([10.251.156.152]) by fmsmga005.fm.intel.com with ESMTP; 14 Apr 2019 01:03:40 -0700 MIME-Version: 1.0 In-Reply-To: <20190412233128.4756-9-lersek@redhat.com> References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-9-lersek@redhat.com> To: Laszlo Ersek , edk2-devel-groups-io From: "Jordan Justen" Cc: Anthony Perard , Ard Biesheuvel , Julien Grall Subject: Re: [edk2-devel] [PATCH 08/10] OvmfPkg: suppress "Value stored to ... is never read" analyzer warnings Message-ID: <155522899517.21857.3170299631470056267@jljusten-skl> User-Agent: alot/0.8 Date: Sun, 14 Apr 2019 01:03:40 -0700 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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. >=20 > For each such warning that has been emitted for OvmfPkg, > the case is one of the following however: >=20 > - We assign a variable a value for (re-)initialization's sake, in > preparation for further or future uses. This practice is safe (sometim= es > even recommended!), hence we should suppress these warnings. >=20 > - We capture a result or a computation in a variable, following a genera= l > programming pattern, but then decide to ignore the value in that > particular case. This is again safe, and we should suppress these > warnings too. >=20 > According to the Clang documentation at >=20 > https://clang-analyzer.llvm.org/faq.html#dead_store >=20 > we should use >=20 > (void)x; >=20 > See the logs below (produced originally for edk2-stable201903). >=20 > > Error: CLANG_WARNING: > > edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:156:3: warning: Value > > stored to 'NumberOfTableEntries' is never read > > # NumberOfTableEntries =3D 0; > > # ^ ~ > > edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:156:3: note: Value > > stored to 'NumberOfTableEntries' is never read > > # NumberOfTableEntries =3D 0; > > # ^ ~ > > # 154| DsdtTable =3D NULL; > > # 155| TableHandle =3D 0; > > # 156|-> NumberOfTableEntries =3D 0; > > # 157| > > # 158| // > > > > Error: CLANG_WARNING: > > edk2-89910a39dcfd/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c:= 43:3: > > warning: Value stored to 'SetupSize' is never read > > # SetupSize =3D 0; > > # ^ ~ > > edk2-89910a39dcfd/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c:= 43:3: > > note: Value stored to 'SetupSize' is never read > > # SetupSize =3D 0; > > # ^ ~ > > # 41| > > # 42| SetupBuf =3D NULL; > > # 43|-> SetupSize =3D 0; > > # 44| KernelBuf =3D NULL; > > # 45| KernelInitialSize =3D 0; > > > > Error: CLANG_WARNING: > > edk2-89910a39dcfd/OvmfPkg/Library/SerializeVariablesLib/SerializeVaria= blesLib.c:609:9: > > warning: Value stored to 'VariableDataBufferSize' is never read > > # VariableDataBufferSize =3D 0; > > # ^ ~ > > edk2-89910a39dcfd/OvmfPkg/Library/SerializeVariablesLib/SerializeVaria= blesLib.c:609:9: > > note: Value stored to 'VariableDataBufferSize' is never read > > # VariableDataBufferSize =3D 0; > > # ^ ~ > > # 607| FreePool (VariableData); > > # 608| VariableData =3D NULL; > > # 609|-> VariableDataBufferSize =3D 0; > > # 610| } > > # 611| VariableData =3D AllocatePool (VariableDataSize); > > > > Error: CLANG_WARNING: > > edk2-89910a39dcfd/OvmfPkg/Library/VirtioLib/VirtioLib.c:125:3: warning= : > > Value stored to 'RingPagesPtr' is never read > > # RingPagesPtr +=3D sizeof *Ring->Used.AvailEvent; > > # ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > edk2-89910a39dcfd/OvmfPkg/Library/VirtioLib/VirtioLib.c:125:3: note: > > Value stored to 'RingPagesPtr' is never read > > # RingPagesPtr +=3D sizeof *Ring->Used.AvailEvent; > > # ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > # 123| > > # 124| Ring->Used.AvailEvent =3D (volatile VOID *) RingPagesPtr; > > # 125|-> RingPagesPtr +=3D sizeof *Ring->Used.AvailEvent; > > # 126| > > # 127| Ring->QueueSize =3D QueueSize; > > > > Error: CLANG_WARNING: > > edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServic= e.c:1079:3: > > warning: Value stored to 'FwhInstance' is never read > > # FwhInstance =3D (EFI_FW_VOL_INSTANCE *) > > # ^ ~~~~~~~~~~~~~~~~~~~~~~~ > > edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServic= e.c:1079:3: > > note: Value stored to 'FwhInstance' is never read > > # FwhInstance =3D (EFI_FW_VOL_INSTANCE *) > > # ^ ~~~~~~~~~~~~~~~~~~~~~~~ > > # 1077| ASSERT_RETURN_ERROR (PcdStatus); > > # 1078| > > # 1079|-> FwhInstance =3D (EFI_FW_VOL_INSTANCE *) > > # 1080| ( > > # 1081| (UINTN) ((UINT8 *) FwhInstance) + FwVolHeader->HeaderL= ength + > > > > Error: CLANG_WARNING: > > edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServic= eDxe.c:173:3: > > warning: Value stored to 'Status' is never read > > # Status =3D gDS->RemoveMemorySpace ( > > # ^ ~~~~~~~~~~~~~~~~~~~~~~~~ > > edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServic= eDxe.c:173:3: > > note: Value stored to 'Status' is never read > > # Status =3D gDS->RemoveMemorySpace ( > > # ^ ~~~~~~~~~~~~~~~~~~~~~~~~ > > # 171| // Mark flash region as runtime memory > > # 172| // > > # 173|-> Status =3D 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 =3D 0; > > # ^ ~ > > edk2-89910a39dcfd/OvmfPkg/SataControllerDxe/SataController.c:231:3: > > note: Value stored to 'DeviceUDmaMode' is never read > > # DeviceUDmaMode =3D 0; > > # ^ ~ > > # 229| UINT16 DeviceUDmaMode; > > # 230| > > # 231|-> DeviceUDmaMode =3D 0; > > # 232| > > # 233| // > > > > Error: CLANG_WARNING: > > edk2-89910a39dcfd/OvmfPkg/VirtioGpuDxe/Gop.c:65:5: warning: Value stor= ed > > to 'Status' is never read > > # Status =3D VirtioGpuSetScanout ( > > # ^ ~~~~~~~~~~~~~~~~~~~~~ > > edk2-89910a39dcfd/OvmfPkg/VirtioGpuDxe/Gop.c:65:5: note: Value stored = to > > 'Status' is never read > > # Status =3D VirtioGpuSetScanout ( > > # ^ ~~~~~~~~~~~~~~~~~~~~~ > > # 63| // by setting ResourceId=3D0 for it. > > # 64| // > > # 65|-> Status =3D 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 +=3D sizeof (UINT16); > > # ^ ~~~~~~~~~~~~~~~ > > edk2-89910a39dcfd/OvmfPkg/VirtioNetDxe/SnpReceive.c:165:3: note: Value > > stored to 'RxPtr' is never read > > # RxPtr +=3D sizeof (UINT16); > > # ^ ~~~~~~~~~~~~~~~ > > # 163| *Protocol =3D (UINT16) ((RxPtr[0] << 8) | RxPtr[1]); > > # 164| } > > # 165|-> RxPtr +=3D sizeof (UINT16); > > # 166| > > # 167| Status =3D EFI_SUCCESS; >=20 > Cc: Anthony Perard > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Julien Grall > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1710 > 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 > --- > 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(+) >=20 > 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 =3D NULL; > Fadt1Table =3D NULL; > Facs2Table =3D NULL; > Facs1Table =3D NULL; > DsdtTable =3D NULL; > TableHandle =3D 0; > NumberOfTableEntries =3D 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 =3D GetXenAcpiRsdp (&XenAcpiRsdpStructurePtr); > if (EFI_ERROR (Status)) { > return Status; > } > > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c b/OvmfP= kg/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 =3D 0; > KernelBuf =3D NULL; > KernelInitialSize =3D 0; > CommandLine =3D NULL; > CommandLineSize =3D 0; > InitrdData =3D NULL; > InitrdSize =3D 0; > > + // > + // Suppress "Value stored to ... is never read" analyzer warnings. > + // > + (VOID)SetupSize; > + > if (!QemuFwCfgIsAvailable ()) { > return EFI_NOT_FOUND; > } > > QemuFwCfgSelectItem (QemuFwCfgItemKernelSize); > KernelSize =3D (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 !=3D 0) { > FreePool (VariableData); > VariableData =3D NULL; > VariableDataBufferSize =3D 0; > + // > + // Suppress "Value stored to ... is never read" analyzer warnin= gs. > + // > + (VOID)VariableDataBufferSize; > } > VariableData =3D AllocatePool (VariableDataSize); > if (VariableData =3D=3D NULL) { > Status =3D EFI_OUT_OF_RESOURCES; > break; > } > VariableDataBufferSize =3D VariableDataSize; > > diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/Vir= tioLib/VirtioLib.c > index 555d2a5ce7c9..0e4b8b6b265e 100644 > --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c > +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c > @@ -113,16 +113,21 @@ VirtioRingInit ( > RingPagesPtr +=3D sizeof *Ring->Used.Idx; > > Ring->Used.UsedElem =3D (volatile VOID *) RingPagesPtr; > RingPagesPtr +=3D sizeof *Ring->Used.UsedElem * QueueSize; > > Ring->Used.AvailEvent =3D (volatile VOID *) RingPagesPtr; > RingPagesPtr +=3D sizeof *Ring->Used.AvailEvent; > > + // > + // Suppress "Value stored to ... is never read" analyzer warnings. > + // > + (VOID)RingPagesPtr; > + > Ring->QueueSize =3D QueueSize; > return EFI_SUCCESS; > } > > > /** > > Tear down the internal resources of a configured virtio ring. > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/O= vmfPkg/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 =3D (EFI_FW_VOL_INSTANCE *) > ( > (UINTN) ((UINT8 *) FwhInstance) + FwVolHeader->HeaderLength + > (sizeof (EFI_FW_VOL_INSTANCE) - sizeof (EFI_FIRMWARE_VOLUME_HEADE= R)) > ); > > + // > + // Suppress "Value stored to ... is never read" analyzer warnings. > + // > + (VOID)FwhInstance; > + > // > // Module type specific hook. > // > InstallVirtualAddressChangeHandler (); > > PcdStatus =3D 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 =3D gDS->RemoveMemorySpace ( > BaseAddress, > Length > ); > > + // > + // Suppress "Value stored to ... is never read" analyzer warnings. > + // > + (VOID)Status; > + > Status =3D gDS->AddMemorySpace ( > EfiGcdMemoryTypeMemoryMappedIo, > BaseAddress, > Length, > EFI_MEMORY_UC | EFI_MEMORY_RUNTIME > ); > ASSERT_EFI_ERROR (Status); > > diff --git a/OvmfPkg/SataControllerDxe/SataController.c b/OvmfPkg/SataCo= ntrollerDxe/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 =3D 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) =3D=3D 0x00) { > return EFI_UNSUPPORTED; > } > > DeviceUDmaMode =3D 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=3D0 for it. > // > Status =3D 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 =3D 0 to disable a scanout." > // > diff --git a/OvmfPkg/VirtioNetDxe/SnpReceive.c b/OvmfPkg/VirtioNetDxe/Sn= pReceive.c > index cdee9a2aee47..51b2f8a4e370 100644 > --- a/OvmfPkg/VirtioNetDxe/SnpReceive.c > +++ b/OvmfPkg/VirtioNetDxe/SnpReceive.c > @@ -153,16 +153,21 @@ VirtioNetReceive ( > } > RxPtr +=3D SIZE_OF_VNET (Mac); > > if (Protocol !=3D NULL) { > *Protocol =3D (UINT16) ((RxPtr[0] << 8) | RxPtr[1]); > } > RxPtr +=3D sizeof (UINT16); > > + // > + // Suppress "Value stored to ... is never read" analyzer warnings. > + // > + (VOID)RxPtr; > + > Status =3D EFI_SUCCESS; > > RecycleDesc: > ++Dev->RxLastUsed; > > // > // virtio-0.9.5, 2.4.1 Supplying Buffers to The Device > // > --=20 > 2.19.1.3.g30247aa5d201 >=20 >=20 >=20 >=20 >=20