From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 15 Apr 2019 09:25:37 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 43F9F2F9468; Mon, 15 Apr 2019 16:25:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-136.rdu2.redhat.com [10.10.121.136]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6611460123; Mon, 15 Apr 2019 16:25:34 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 08/10] OvmfPkg: suppress "Value stored to ... is never read" analyzer warnings To: Jordan Justen , edk2-devel-groups-io Cc: Anthony Perard , Ard Biesheuvel , Julien Grall References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-9-lersek@redhat.com> <155522899517.21857.3170299631470056267@jljusten-skl> From: "Laszlo Ersek" Message-ID: Date: Mon, 15 Apr 2019 18:25:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <155522899517.21857.3170299631470056267@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Mon, 15 Apr 2019 16:25:36 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Julien Grall >> 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 >> --- >> 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. 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!) Thanks! Laszlo >> // >> // 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 >> >> >> >> >>