Re: [PATCH] misra: add R14.4 R21.1 R21.2

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 25 Oct 2023, Jan Beulich wrote:
> On 25.10.2023 03:15, Stefano Stabellini wrote:
> > On Tue, 24 Oct 2023, Jan Beulich wrote:
> >> On 24.10.2023 01:31, Stefano Stabellini wrote:> --- a/docs/misra/rules.rst
> >>> +++ b/docs/misra/rules.rst
> >>> @@ -422,6 +422,13 @@ maintainers if you want to suggest a change.
> >>>  
> >>>         while(0) and while(1) and alike are allowed.
> >>>  
> >>> +   * - `Rule 14.4 
> >>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_14_04.c>`_
> >>> +     - Required
> >>> +     - The controlling expression of an if statement and the controlling
> >>> +       expression of an iteration-statement shall have essentially
> >>> +       Boolean type
> >>> +     - Implicit conversions to boolean are allowed
> >>
> >> What, if anything, remains of this rule with this exception?
> > 
> > Not much, but there is a difference between following a rule with a
> > deviation (in this case we allow implicit conversions of integers and
> > pointers to bool because we claim all Xen developers know how they work)
> > and not follow the rule at all, at least for the assessors. Also,
> > anything that doesn't implicitly convert to a boolean is not allowed,
> > although I guess probably it wouldn't compile either.
> > 
> > We could also try to find a better wording to reduce the deviation only
> > to integer and pointers. Any suggestions?
> 
> Since compound types can't be converted anyway, I think only floating
> point types (and their relatives) remain, which we don't use anyway
> (and if we did, excepting them as well would only be logical imo). I
> therefore see little point in making "integers and pointers" explicit.
> 
> Instead I wonder if we shouldn't make ourselves honest and say we
> deviate this rule as a whole.

Yes, I see your point. I think I'll remove Rule 14.4 from the patch and
put it in the bucket of all the rules deviated as a whole (which we
should track as a separate rst file but today we don't.)


> >>> @@ -479,6 +486,24 @@ maintainers if you want to suggest a change.
> >>>         they are related
> >>>       -
> >>>  
> >>> +   * - `Rule 21.1 
> >>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_01.c>`_
> >>> +     - Required
> >>> +     - #define and #undef shall not be used on a reserved identifier or
> >>> +       reserved macro name
> >>> +     - No identifiers should start with _BUILTIN to avoid clashes with
> >>
> >> DYM "__builtin_"? Also gcc introduces far more than just __builtin_...()
> >> into the name space.
> > 
> > Yes agreed, but in my notes that is the only one I wrote down. I recall
> > that the complete list is a couple of pages long, I don't think we can
> > possibly add it here in full and if I recall it is not available in the
> > GCC documentation. More on this below.
> > 
> > 
> >>> +       GCC reserved identifiers. In general, all identifiers starting 
> >>> with
> >>> +       an underscore are reserved, and are best avoided.
> >>
> >> This isn't precise enough. A leading underscore followed by a lower-case
> >> letter or a number is okay to use for file-scope symbols. Imo we should
> >> not aim at removing such uses, but rather encourage more use.
> > 
> > More on this below
> > 
> > 
> >> In this context, re-reading some of the C99 spec, I have to realize that
> >> my commenting on underscore-prefixed macro parameters (but not underscore-
> >> prefixed locals in macros) was based on ambiguous information in the spec.
> >> I will try to remember to not comment on such anymore going forward.
> >>
> >>> However, Xen
> >>> +       makes extensive usage of leading underscores in header guards,
> >>> +       bitwise manipulation functions, and a few other places. They are
> >>> +       considered safe as checks have been done against the list of
> >>> +       GCC's reserved identifiers. They don't need to be replaced.
> >>
> >> This leaves quite vague what wants and what does not want replacing, nor
> >> what might be okay to introduce subsequently despite violation this rule.
> >  
> > My goals here were to convey the following:
> > 1) avoid clashes with gcc reserved identifiers
> > 2) in general try to reduce the usage of leading underscores except for
> >    known existing locations (header guards, bitwise manipulation
> >    functions)
> > 
> > However, for 1) the problem is that we don't have the full list and for
> > 2) the problem is that it is too vague.
> > 
> > Instead I suggest we do the following:
> > - we get the full list of gcc reserved identifiers from Roberto and we
> >   commit it to xen.git as a seprate file under docs/misra
> 
> Such a full list can only be compiled for any specific gcc version. Even
> non-upstream backports by a distro may alter this list.
> 
> > - we reference the list here saying one should avoid clashes with those
> >   identifiers as reserved by gcc
> 
> With the list constantly changing (mostly expanding), that's pretty
> hopeless.
> 
> > And if we can find a clear general comment about the usage of leading
> > underscores in Xen I am happy to add it too (e.g. header guards), but
> > from MISRA point of view the above is sufficient.
> 
> But what we need isn't a description of the status quo, but one of
> what state we want to (slowly) move to. The status quo anyway is
> "no pattern, as in the past hardly anyone cared".

I guess you are suggesting something more like the following, but please
feel free to suggest your favorite wording (it might be easier for me to
understand what you mean if you provide a short example).

---
All identifiers starting with an underscore are reserved and should not
be used. Today Xen uses many, such as header guards and bitwise
manipulation functions. Upon analysis it turns out Xen identifiers do
not clash with the identifiers used by modern GCC, but that is not a
guarantee that there won't be a naming clash in the future or with
another compiler. For these reasons we discourage the introduction of
new reserved identifiers in Xen, and we see it as positive the reduction
of reserved identifiers. At the same time, certain identifiers starting
with an underscore are also commonly used in Linux (e.g. __set_bit) and
we don't think it would be an improvement to rename them.