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 <af...@apple.com> >> Cc: Leif Lindholm <leif.lindh...@linaro.org> >> Cc: Michael D Kinney <michael.d.kin...@intel.com> >> Cc: Rebecca Cran <rebe...@bsdio.com> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=607 >> Signed-off-by: Laszlo Ersek <ler...@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 >> | | >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47037): https://edk2.groups.io/g/devel/message/47037 Mute This Topic: https://groups.io/mt/33157544/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
