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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 9/9/19 2:25 PM, Laszlo Ersek wrote:
> 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.

Oh, I see, this issue is old; I was not aware of EDK2 existence when it
was discussed.

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

OK I understand.

Your patch is an improvement regarding what we have today, enforcing a
cleaner codebase, so:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

I still wonder how many people rejected the macro proposal and agreed to
add comments instead, and why...
>From both my developer/reviewer point of view, the macros are obvious
and self-documented. Eventually we can restart the discussion regarding
using macros, and later use them. I'm not sure this is the best use of
our time.

Regards,

Phil.

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