Re: [edk2-devel] [PATCH edk2-CCSS 3/3] must comment: add rule for documenting spurious variable assignments

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

On 09/06/19 10:13, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> (Cc'ing Ard)
> 
> On 9/5/19 8:38 PM, Laszlo Ersek wrote:
>> Problem statement from Ard:
>>
>>> Sometimes, the GCC compiler warns about variables potentially being used
>>> without having been initialized, while visual inspection reveals that
>>> this is impossible. In such cases, we need to initialize such a variable
>>> to an arbitrary value only to avoid breaking the build, given our policy
>>> to treat warnings as errors.
> 
> This is annoying.
> 
> I suppose using CFLAGS+='-Wno-uninitialized -Wmaybe-uninitialized' is
> not an acceptable option.

I don't have links handy, but around or before the time I filed
TianoCore#607, we had gone through all the possibilities. The issue may
have been possible to suppress with cmdline options for a particular
toolchain version, but I'm fairly sure it was impossible to solve for
all the toolchains simultaneously that edk2 supported at the time.

>>
>> In such cases we generally use
>>
>>   LocalIntegerVariable = 0;
>>
>> and
>>
>>   LocalPointerVariable = NULL;
>>
>> which takes care of the incorrect warning. However, it also makes the
>> human analysis of any subsequent logic harder, because it suggests that
>> assigning that specific zero or NULL value to the local variable is
>> *required* by the subsequent logic.
> 
> What about having explicit definitions to silent warnings, so we don't
> need to add comments?
> 
> #define UNINITIALIZED_INTEGER 0
> #define UNINITIALIZED_POINTER NULL
> 
> Human review becomes trivial:
> 
>    LocalPointerVariable = UNINITIALIZED_POINTER;

We did consider macros too, if I remember correctly. It was not liked.
(We definitely considered magic values, see 0xDEADBEEF below, and those
were clearly rejected.) People really seemed to want zero / NULL values,
open-coded. I disagreed, but accepted. The explicit comment suggestion
was a compromise from my side, therefore.

In this patch set, I wouldn't like to introduce a rule that is not based
in current practice. The code base is already full of the above kind of
zero / NULL assignment; the only coding style detail, from the rule
being suggested, is the comment.

While TianoCore#607 has been open, I've consistently directed developers
to it, for the proposed syntax. Therefore, if you look at the code base
today, you will find a large amount of the original un-annotated zero /
NULL assignment (where you can't immediately tell whether they are
algorithmically necessery or not), and a few instances of the wording
proposed here.

$ git grep 'incorrect compiler/analyzer'

In that regard, this patch set aims to codify existing practice -- I
just want to make the pattern more consistent.

Thanks
Laszlo

> 
>> In order to highlight such assignments, whose sole purpose is to suppress
>> invalid "use before init" warnings from compilers or static analysis
>> tools, we should mandate comments such as:
>>
>>   //
>>   // set LocalVariable to suppress incorrect compiler/analyzer warnings
>>   //
>>   LocalVariable = 0;
>>
>> (Magic values such as 0xDEADBEEF, which would obviate the necessity of
>> explicit comments, have been considered, and rejected for stylistic
>> reasons.)
>>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  6_documenting_software/64_what_you_must_comment.md | 39 ++++++++++++++++++++
>>  README.md                                          |  1 +
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/6_documenting_software/64_what_you_must_comment.md b/6_documenting_software/64_what_you_must_comment.md
>> index abb2114bf5bc..9e51c2e45816 100644
>> --- a/6_documenting_software/64_what_you_must_comment.md
>> +++ b/6_documenting_software/64_what_you_must_comment.md
>> @@ -58,3 +58,42 @@ instance differs.
>>  
>>  When possible, you should also list the requirements that are satisfied by the
>>  code.
>> +
>> +### 6.4.6 Comment spurious variable assignments.
>> +
>> +A compiler or static code analyzer may warn that an object with automatic or
>> +allocated storage duration is read without having been initialized, while
>> +visual inspection reveals that this is impossible.
>> +
>> +In order to suppress such a warning (which is emitted due to invalid data flow
>> +analysis), developers explicitly assign the affected object the value to which
>> +the same object would be initialized automatically, had the object static
>> +storage duration, and no initializer. (The value assigned could be arbitrary;
>> +the above-mentioned value is chosen for stylistic reasons.) For example:
>> +
>> +```c
>> +UINTN LocalIntegerVariable;
>> +VOID  *LocalPointerVariable;
>> +
>> +LocalIntegerVariable = 0;
>> +LocalPointerVariable = NULL;
>> +```
>> +
>> +This kind of assignment is difficult to distinguish from assignments where the
>> +initial value of an object is meaningful, and is consumed by other code without
>> +an intervening assignment. Therefore, each such assignment must be documented,
>> +as follows:
>> +
>> +```c
>> +UINTN LocalIntegerVariable;
>> +VOID  *LocalPointerVariable;
>> +
>> +//
>> +// set LocalIntegerVariable to suppress incorrect compiler/analyzer warnings
>> +//
>> +LocalIntegerVariable = 0;
>> +//
>> +// set LocalPointerVariable to suppress incorrect compiler/analyzer warnings
>> +//
>> +LocalPointerVariable = NULL;
>> +```
>> diff --git a/README.md b/README.md
>> index e26133540368..0648819f0d3a 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -113,3 +113,4 @@ Copyright (c) 2006-2017, Intel Corporation. All rights reserved.
>>  | 2.2      | Convert to Gitbook                                                                                                                                | June 2017  |
>>  |          | [#425](https://bugzilla.tianocore.org/show_bug.cgi?id=425) [CCS] clarify line breaking and indentation requirements for multi-line function calls |            |
>>  |          | [#1656](https://bugzilla.tianocore.org/show_bug.cgi?id=1656) Update all Wiki pages for the BSD+Patent license change with SPDX identifiers        |            |
>> +|          | [#607](https://bugzilla.tianocore.org/show_bug.cgi?id=607) Document code comment requirements for spurious variable assignments                   |            |
>>