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 <phi...@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 <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 (#47039): https://edk2.groups.io/g/devel/message/47039 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] -=-=-=-=-=-=-=-=-=-=-=-
