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.24, mailfrom: jordan.l.justen@intel.com) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by groups.io with SMTP; Tue, 16 Apr 2019 02:26:03 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Apr 2019 02:26:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,357,1549958400"; d="scan'208";a="316372873" Received: from mjguyett-mobl1.amr.corp.intel.com (HELO localhost) ([10.251.132.204]) by orsmga005.jf.intel.com with ESMTP; 16 Apr 2019 02:26:01 -0700 MIME-Version: 1.0 In-Reply-To: References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-9-lersek@redhat.com> <155522899517.21857.3170299631470056267@jljusten-skl> To: Laszlo Ersek , edk2-devel-groups-io From: "Jordan Justen" Subject: Re: [edk2-devel] [PATCH 08/10] OvmfPkg: suppress "Value stored to ... is never read" analyzer warnings Cc: Anthony Perard , Ard Biesheuvel , Julien Grall Message-ID: <155540676133.13612.706923400033532497@jljusten-skl> User-Agent: alot/0.8 Date: Tue, 16 Apr 2019 02:26:01 -0700 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 (somet= imes > >> even recommended!), hence we should suppress these warnings. > >> > >> - We capture a result or a computation in a variable, following a gene= ral > >> 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 =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/SerializeVari= ablesLib.c:609:9: > >>> warning: Value stored to 'VariableDataBufferSize' is never read > >>> # VariableDataBufferSize =3D 0; > >>> # ^ ~ > >>> edk2-89910a39dcfd/OvmfPkg/Library/SerializeVariablesLib/SerializeVari= ablesLib.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: warnin= g: > >>> 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/FwBlockServi= ce.c:1079:3: > >>> warning: Value stored to 'FwhInstance' is never read > >>> # FwhInstance =3D (EFI_FW_VOL_INSTANCE *) > >>> # ^ ~~~~~~~~~~~~~~~~~~~~~~~ > >>> edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServi= ce.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->Header= Length + > >>> > >>> Error: CLANG_WARNING: > >>> edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServi= ceDxe.c:173:3: > >>> warning: Value stored to 'Status' is never read > >>> # Status =3D gDS->RemoveMemorySpace ( > >>> # ^ ~~~~~~~~~~~~~~~~~~~~~~~~ > >>> edk2-89910a39dcfd/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServi= ceDxe.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 sto= red > >>> 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; > >> > >> 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(+) > >> > >> diff --git a/OvmfPkg/AcpiPlatformDxe/Xen.c b/OvmfPkg/AcpiPlatformDxe/X= en.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; > >> =20 > >> + // > >> + // Suppress "Value stored to ... is never read" analyzer warnings. > >> + // > >> + (VOID)NumberOfTableEntries; > >> + > >=20 > > I've seen this solution to shut up the compiler for similar reasons, > > and it kind of bugs me. > >=20 > > It looks like both paths of the if & else initialize > > NumberOfTableEntries, so maybe we can just drop setting it to 0? >=20 > It might trigger *other* compilers to whine about "variable read without > initialization" :) >=20 > > I looked at FwhInstance too, and it doesn't look like it is used after > > being set. So, maybe that should just be dropped? > >=20 > > I guess both of your bullet points give arguments allowing the unused > > set. > >=20 > > Rather than adding "Suppress..." comments everywhere, maybe a global > > macro could be defined for similar uses in EDK II? > >=20 > > // > > // Suppress "Value stored to ... is never read" analyzer warnings. > > // > > #define IGNORE_UNUSED_ASSIGNMENT(var) ((VOID)var) > >=20 > > Then again, I guess we decided to suppress this in EDK II compiler > > warnings with -Wno-unused-but-set-variable. > >=20 > > So, can you adjust the RH covscan tool settings to similarly ignore > > these warnings? >=20 > 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 wit= h /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