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; Tue, 16 Apr 2019 04:44:57 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 533014E92B; Tue, 16 Apr 2019 11:44:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-171.rdu2.redhat.com [10.10.120.171]) by smtp.corp.redhat.com (Postfix) with ESMTP id ADF805C1B4; Tue, 16 Apr 2019 11:44:54 +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> <155540676133.13612.706923400033532497@jljusten-skl> From: "Laszlo Ersek" Message-ID: <66388f6f-2748-862a-5654-b7e8e35883c7@redhat.com> Date: Tue, 16 Apr 2019 13:44:53 +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: <155540676133.13612.706923400033532497@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 16 Apr 2019 11:44:57 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/16/19 11:26, Jordan Justen wrote: > 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 >>>> 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. > > 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. Can you please go through the series one by one, and suggest individually which patches should be dropped? >> 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 ) It's great when warnings can be tweaked with such resolution, at the category level. There are two counter-arguments. - Regarding "potentially uninitialized local variable", we could have globally disabled "-Wmaybe-uninitialized" under GCC. The decision not to do that was conscious -- apparently we prefer adding the zero-assignments manually, and getting a few valid warnings in exchange, to not getting any such warnings (valid or invalid). We only suppress the warning for external projects (OpenSSL and Oniguruma). Admittedly, it's not clear to me why the VS2013/VS2015 approach *differs* from the GCC approach -- but that too could be justified, if VS emitted an unbearable amount of false positives. I'm not sure. - The other argument against such category-level tweaking is that clang's *analyzer* (not the base compiler) simply doesn't offer it, according to its documentation. (Please see more details below.) So here's my opinion on each patch: > 1 5b84fae949ec MdePkg/PiFirmwareFile: express IS_SECTION2 in terms of SECTION_SIZE Just a refactoring, so that the next patch fixes IS_SECTION2() at once. We should keep it. > 2 937afaeb7349 MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE I think we should keep this, as-is. The warning is justified, and IMO the patch uses well-defined behavior. > 3 23c61ae8b546 BaseTools/PiFirmwareFile: fix undefined behavior in SECTION_SIZE Reflects the previous fix to BaseTools; we should keep it. > 4 f1ff9add5f9e MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE Valid warning, we should keep the patch. > 5 28ec922c05b3 OvmfPkg/Sec: fix out-of-bounds reads Valid warning, fix the issue by using the now-fixed SECTION_SIZE / FFS_FILE_SIZE. > 6 474482564fee OvmfPkg/QemuVideoDxe: avoid arithmetic on null pointer Valid warning, we should keep the patch. > 7 28d72e8d441e OvmfPkg/AcpiPlatformDxe: suppress invalid "deref of undef pointer" warning False positive from clang-analyzer. - As I wrote in the commit message, the direct report ("Dereference of undefined pointer value") can be suppressed, as first step, with a NULL assignment. This is something we tend to do anyway, because edk2 toolchains as well report such false positives occasionally. However, the NULL-assignment only changes the report to "Dereference of null pointer". (See a similar example here: .) - The latter warning (nullptr deref) is addressed by the clang-analyzer FAQ: I'm fine dropping this patch (and suppressing it, based on source code location, in RH covscan), but I don't think clang-analyzer itself offers any pragma or option to suppress the warning category. > 8 46ee37494741 OvmfPkg: suppress "Value stored to ... is never read" analyzer warnings I would *definitely* suppress this warning globally, if clang-analyzer made that possible. It's not possible however: . It only talks about "specific" dead stores, and ((void)x) as a workaround. See also: - https://clang-analyzer.llvm.org/faq.html#suppress_issue - https://clang-analyzer.llvm.org/faq.html#exclude_code > Q: How can I suppress a specific analyzer warning? > > There is currently no solid mechanism for suppressing an analyzer > warning, although this is currently being investigated. When you > encounter an analyzer bug/false positive, check if it's one of the > issues discussed above or if the analyzer annotations can resolve the > issue. Second, please report it to help us improve user experience. As > the last resort, consider using __clang_analyzer__ macro described > below. > > Q: How can I selectively exclude code the analyzer examines? > > When the static analyzer is using clang to parse source files, it > implicitly defines the preprocessor macro __clang_analyzer__. One can > use this macro to selectively exclude code the analyzer examines. Here > is an example: > > #ifndef __clang_analyzer__ > // Code not to be analyzed > #endif > > This usage is discouraged because it makes the code dead to the > analyzer from now on. Instead, we prefer that users file bugs against > the analyzer when it flags false positives. Regarding annotations , nothing seems to apply, for suppressing dead store warnings generally. All in all, I don't think it's possible to implement your recommendation, in either build flags, or in "Base.h". But, again, that doesn't mean I want to keep this patch. I'm fine dropping it an suppressing the warning in another component (in RH covscan). > 9 5a48ea70c227 OvmfPkg/AcpiPlatformDxe: catch theoretical nullptr deref in Xen code This patch should be kept, as the warning is valid. > 10 d10d8364fe7f OvmfPkg/BasePciCapLib: suppress invalid "nullptr deref" warning False positive, we can drop the patch. I can deal with the removal (and possibly with the reworking) of individual patches; what I can not deal with is general / sweeping control over clang-analyzer's warning categories. The clang-analyzer project appears opposed to that approach (based on their FAQ). Please mark patches individually that you'd like me to drop. Thanks! Laszlo