Re: [edk2-devel] [PATCH 08/10] OvmfPkg: suppress "Value stored to ... is never read" analyzer warnings

["Laszlo Ersek" <lersek@redhat.com>]

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 <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.

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:
  <https://bugzilla.mozilla.org/show_bug.cgi?id=712497>.)

- The latter warning (nullptr deref) is addressed by the clang-analyzer
  FAQ: <https://clang-analyzer.llvm.org/faq.html#null_pointer>

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:
<https://clang-analyzer.llvm.org/faq.html#dead_store>. 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
<https://clang-analyzer.llvm.org/annotations.html>, 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