From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: philmd@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 09 Sep 2019 06:35:19 -0700 Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 07E5C3DE31 for ; Mon, 9 Sep 2019 13:35:19 +0000 (UTC) Received: by mail-wm1-f71.google.com with SMTP id m6so4485374wmf.2 for ; Mon, 09 Sep 2019 06:35:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=HyqTMte2XQIG65OtdIGaVx+K/QZxhLo8kS0qYfs/b2Y=; b=RLHMk4+rpmUHxNf3p1d7FeRr4DMWeI0Mt77hFUS6fF2tDMFAd/6wfpTzWpZfc/ZfOu YcmUq9VzhtJqGrY8c3y2dlAWYFPXMYKLk0pns2ASF++Gca80K9d0AX2vbnMuoKPgSscr Vf1tQlbDYn7SNF1eD944HlEdmu1g5gXZ3hPJ509UnwOyRThqAikeRC7Zv1lSpe1GY26c KH+rJtHxuI3C/Kb8L5oYFQUpkHWn2+5N15wBzSpbnFNJMSQ/Dh+JnLuSbzueCVBT79sc llJvqylltFBvWDxUfCZepbaS0T2To3NE5S6U+zQ2w/30hJtQTZLd/w5Gfr2U3f1YbE6k yAnw== X-Gm-Message-State: APjAAAXqIVZfqSj9+ufZg9xDdU7O+pw3kPU3HCEdA0NuoxK/SPt41vcL Sph7jGDKepiISFOnsq4ArBP+9jZPemixXXtwL6HeAu5nrNNqPsSsUte4MKHvgs56IfsJOTekEzA HlqruZjDP+P7hrQ== X-Received: by 2002:a05:6000:142:: with SMTP id r2mr19209935wrx.212.1568036117773; Mon, 09 Sep 2019 06:35:17 -0700 (PDT) X-Google-Smtp-Source: APXvYqwCVZAMtYpLbCtdgeLs5iyieR0FwCaIQquEs/G/IDm5aL+MSCXRm6Gs56LrwDho27Fg4tvNag== X-Received: by 2002:a05:6000:142:: with SMTP id r2mr19209911wrx.212.1568036117565; Mon, 09 Sep 2019 06:35:17 -0700 (PDT) Received: from [192.168.1.41] (251.red-88-10-102.dynamicip.rima-tde.net. [88.10.102.251]) by smtp.gmail.com with ESMTPSA id n7sm13926991wrx.42.2019.09.09.06.35.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 09 Sep 2019 06:35:17 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH edk2-CCSS 3/3] must comment: add rule for documenting spurious variable assignments To: Laszlo Ersek , devel@edk2.groups.io Cc: Andrew Fish , Leif Lindholm , Michael D Kinney , Rebecca Cran , Ard Biesheuvel References: <20190905183820.10312-1-lersek@redhat.com> <20190905183820.10312-4-lersek@redhat.com> <5d2526af-ed97-2267-b820-08deb064a278@redhat.com> <1b1859ce-820e-d6c7-111e-16932c13e4c6@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: <0c159eed-2b79-e3fd-61c0-8b7396338c0c@redhat.com> Date: Mon, 9 Sep 2019 15:35:16 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <1b1859ce-820e-d6c7-111e-16932c13e4c6@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 9/9/19 2:25 PM, Laszlo Ersek wrote: > On 09/06/19 10:13, Philippe Mathieu-Daud=C3=A9 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 tha= t >>>> this is impossible. In such cases, we need to initialize such a vari= able >>>> to an arbitrary value only to avoid breaking the build, given our po= licy >>>> to treat warnings as errors. >> >> This is annoying. >> >> I suppose using CFLAGS+=3D'-Wno-uninitialized -Wmaybe-uninitialized' i= s >> not an acceptable option. >=20 > 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 =3D 0; >>> >>> and >>> >>> LocalPointerVariable =3D NULL; >>> >>> which takes care of the incorrect warning. However, it also makes the >>> human analysis of any subsequent logic harder, because it suggests th= at >>> 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 =3D UNINITIALIZED_POINTER; >=20 > 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 base= d > 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. >=20 > While TianoCore#607 has been open, I've consistently directed developer= s > 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. >=20 > $ git grep 'incorrect compiler/analyzer' >=20 > 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 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 supp= ress >>> invalid "use before init" warnings from compilers or static analysis >>> tools, we should mandate comments such as: >>> >>> // >>> // set LocalVariable to suppress incorrect compiler/analyzer warnin= gs >>> // >>> LocalVariable =3D 0; >>> >>> (Magic values such as 0xDEADBEEF, which would obviate the necessity o= f >>> explicit comments, have been considered, and rejected for stylistic >>> reasons.) >>> >>> Cc: Andrew Fish >>> Cc: Leif Lindholm >>> Cc: Michael D Kinney >>> Cc: Rebecca Cran >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D607 >>> Signed-off-by: Laszlo Ersek >>> --- >>> 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_d= ocumenting_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. >>> =20 >>> When possible, you should also list the requirements that are satisf= ied by the >>> code. >>> + >>> +### 6.4.6 Comment spurious variable assignments. >>> + >>> +A compiler or static code analyzer may warn that an object with auto= matic 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 valu= e to which >>> +the same object would be initialized automatically, had the object s= tatic >>> +storage duration, and no initializer. (The value assigned could be a= rbitrary; >>> +the above-mentioned value is chosen for stylistic reasons.) For exam= ple: >>> + >>> +```c >>> +UINTN LocalIntegerVariable; >>> +VOID *LocalPointerVariable; >>> + >>> +LocalIntegerVariable =3D 0; >>> +LocalPointerVariable =3D 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 c= ode without >>> +an intervening assignment. Therefore, each such assignment must be d= ocumented, >>> +as follows: >>> + >>> +```c >>> +UINTN LocalIntegerVariable; >>> +VOID *LocalPointerVariable; >>> + >>> +// >>> +// set LocalIntegerVariable to suppress incorrect compiler/analyzer = warnings >>> +// >>> +LocalIntegerVariable =3D 0; >>> +// >>> +// set LocalPointerVariable to suppress incorrect compiler/analyzer = warnings >>> +// >>> +LocalPointerVariable =3D 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 r= ights reserved. >>> | 2.2 | Convert to Gitbook = = | June 2017 | >>> | | [#425](https://bugzilla.tianocore.org/show_bug.cgi?id=3D= 425) [CCS] clarify line breaking and indentation requirements for multi-l= ine function calls | | >>> | | [#1656](https://bugzilla.tianocore.org/show_bug.cgi?id=3D= 1656) Update all Wiki pages for the BSD+Patent license change with SPDX i= dentifiers | | >>> +| | [#607](https://bugzilla.tianocore.org/show_bug.cgi?id=3D= 607) Document code comment requirements for spurious variable assignments= | | >>> >=20