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

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

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.

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

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