[PATCH 0/2] automation/eclair_analysis: deviate MISRA C Rule 21.2

[PATCH 0/2] automation/eclair_analysis: deviate MISRA C Rule 21.2

[Alessandro Zucchelli]

This series aims to address several violations of Rule 21.2 which states the
following: A reserved identifier or reserved macro name shall not be declared.
The series contains two patches, one changes x86/APIC which used an identifier
starting with '__', the second deviates all reserved identifiers with the 
exception of those starting with "__builtin_" which still remain available.

Alessandro Zucchelli (1):
  automation/eclair_analysis: deviate MISRA C Rule 21.2

Nicola Vetrini (1):
  x86/APIC: address violation of MISRA C Rule 21.2

 automation/eclair_analysis/ECLAIR/deviations.ecl | 11 +++++++++++
 xen/arch/x86/apic.c                              |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] automation/eclair_analysis: deviate MISRA C Rule 21.2

[Alessandro Zucchelli]

Rule 21.2 reports identifiers reserved for the C and POSIX standard
libraries: all xen's translation units are compiled with option
-nostdinc, this guarantees that these libraries are not used, therefore
a justification is provided for allowing uses of such identifiers in
the project.
Builtins starting with "__builtin_" still remain available.

No functional change.

Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 447c1e6661..9fa9a7f01c 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -487,6 +487,17 @@ leads to a violation of the Rule are deviated."
 # Series 21.
 #
 
+-doc_begin="Rules 21.1 and 21.2 report identifiers reserved for the C and POSIX
+standard libraries: if these libraries are not used there is no reason to avoid such
+identifiers. All xen's translation units are compiled with option -nostdinc,
+this guarantees that these libraries are not used. Some compilers could perform
+optimization using built-in functions: this risk is partially addressed by
+using the compilation option -fno-builtin. Builtins starting with \"__builtin_\"
+still remain available."
+-config=MC3R1.R21.1,macros={safe , "!^__builtin_$" }
+-config=MC3R1.R21.2,declarations+={safe, "!^__builtin_.*$"}
+-doc_end
+
 -doc_begin="Xen does not use the functions provided by the Standard Library, but
 implements a set of functions that share the same names as their Standard Library equivalent.
 The implementation of these functions is available in source form, so the undefined, unspecified
-- 
2.34.1

Re: [PATCH 1/2] automation/eclair_analysis: deviate MISRA C Rule 21.2

[Jan Beulich]

On 19.06.2024 19:09, Alessandro Zucchelli wrote:
> Rule 21.2 reports identifiers reserved for the C and POSIX standard
> libraries: all xen's translation units are compiled with option
> -nostdinc, this guarantees that these libraries are not used, therefore
> a justification is provided for allowing uses of such identifiers in
> the project.
> Builtins starting with "__builtin_" still remain available.
> 
> No functional change.
> 
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index 447c1e6661..9fa9a7f01c 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -487,6 +487,17 @@ leads to a violation of the Rule are deviated."
>  # Series 21.
>  #
>  
> +-doc_begin="Rules 21.1 and 21.2 report identifiers reserved for the C and POSIX
> +standard libraries: if these libraries are not used there is no reason to avoid such
> +identifiers. All xen's translation units are compiled with option -nostdinc,
> +this guarantees that these libraries are not used. Some compilers could perform
> +optimization using built-in functions: this risk is partially addressed by
> +using the compilation option -fno-builtin. Builtins starting with \"__builtin_\"
> +still remain available."

While the sub-section "Reserved Identifiers" is part of Section 7,
"Library", close coordination is needed between the library and the
compiler, which only together form an "implementation". Therefore any
use of identifiers beginning with two underscores or beginning with an
underscore and an upper case letter is at risk of colliding not only
with a particular library implementation (which we don't use), but
also of such with a particular compiler implementation (which we cannot
avoid to use). How can we permit use of such potentially problematic
identifiers?

Further, as to the rule mentioning file scope identifiers: Why is that?
The text in the C99 specification does not preclude their use, it merely
restricts what they may be used for. Why does Misra go yet farther?

Finally, why "partially addressed"? What part is unaddressed?

> +-config=MC3R1.R21.1,macros={safe , "!^__builtin_$" }
> +-config=MC3R1.R21.2,declarations+={safe, "!^__builtin_.*$"}

First: Why the differences in = vs += and in absence vs presence of .*

Second: The rules, according to my understanding, are about us defining
or declaring such identifiers, not about us using them. There shouldn't
be any #define, #undef, or declaration (let alone definition) of such
entities. All we do is _use_ them as e.g. expansion of #define-s. Thus:
Why is a deviation needed here in the first place? Then again - maybe
I'm reading this wrong, especially the leading ! may perhaps be some
form of negation.

Jan

Re: [PATCH 1/2] automation/eclair_analysis: deviate MISRA C Rule 21.2

[Stefano Stabellini]

On Thu, 20 Jun 2024, Jan Beulich wrote:
> On 19.06.2024 19:09, Alessandro Zucchelli wrote:
> > Rule 21.2 reports identifiers reserved for the C and POSIX standard
> > libraries: all xen's translation units are compiled with option
> > -nostdinc, this guarantees that these libraries are not used, therefore
> > a justification is provided for allowing uses of such identifiers in
> > the project.
> > Builtins starting with "__builtin_" still remain available.
> > 
> > No functional change.
> > 
> > Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> > ---
> >  automation/eclair_analysis/ECLAIR/deviations.ecl | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > index 447c1e6661..9fa9a7f01c 100644
> > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > @@ -487,6 +487,17 @@ leads to a violation of the Rule are deviated."
> >  # Series 21.
> >  #
> >  
> > +-doc_begin="Rules 21.1 and 21.2 report identifiers reserved for the C and POSIX
> > +standard libraries: if these libraries are not used there is no reason to avoid such
> > +identifiers. All xen's translation units are compiled with option -nostdinc,
> > +this guarantees that these libraries are not used. Some compilers could perform
> > +optimization using built-in functions: this risk is partially addressed by
> > +using the compilation option -fno-builtin. Builtins starting with \"__builtin_\"
> > +still remain available."
> 
> While the sub-section "Reserved Identifiers" is part of Section 7,
> "Library", close coordination is needed between the library and the
> compiler, which only together form an "implementation". Therefore any
> use of identifiers beginning with two underscores or beginning with an
> underscore and an upper case letter is at risk of colliding not only
> with a particular library implementation (which we don't use), but
> also of such with a particular compiler implementation (which we cannot
> avoid to use). How can we permit use of such potentially problematic
> identifiers?

Alternative question: is there a way we can check if there is clash of
some sort between a compiler implementation of something and a MACRO or
identifier we have in Xen? An error or a warning from the compiler for
instance? That could be an easy way to prove we are safe.

Also, can we use the fact that the compiler we use is the same compiler
used to compile Linux, and Linux makes extensive use of identifiers and
macros starting with underscores as one of the reason for being safe
from clashes?

Re: [PATCH 1/2] automation/eclair_analysis: deviate MISRA C Rule 21.2

[Jan Beulich]

On 21.06.2024 03:02, Stefano Stabellini wrote:
> On Thu, 20 Jun 2024, Jan Beulich wrote:
>> On 19.06.2024 19:09, Alessandro Zucchelli wrote:
>>> Rule 21.2 reports identifiers reserved for the C and POSIX standard
>>> libraries: all xen's translation units are compiled with option
>>> -nostdinc, this guarantees that these libraries are not used, therefore
>>> a justification is provided for allowing uses of such identifiers in
>>> the project.
>>> Builtins starting with "__builtin_" still remain available.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
>>> ---
>>>  automation/eclair_analysis/ECLAIR/deviations.ecl | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> index 447c1e6661..9fa9a7f01c 100644
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -487,6 +487,17 @@ leads to a violation of the Rule are deviated."
>>>  # Series 21.
>>>  #
>>>  
>>> +-doc_begin="Rules 21.1 and 21.2 report identifiers reserved for the C and POSIX
>>> +standard libraries: if these libraries are not used there is no reason to avoid such
>>> +identifiers. All xen's translation units are compiled with option -nostdinc,
>>> +this guarantees that these libraries are not used. Some compilers could perform
>>> +optimization using built-in functions: this risk is partially addressed by
>>> +using the compilation option -fno-builtin. Builtins starting with \"__builtin_\"
>>> +still remain available."
>>
>> While the sub-section "Reserved Identifiers" is part of Section 7,
>> "Library", close coordination is needed between the library and the
>> compiler, which only together form an "implementation". Therefore any
>> use of identifiers beginning with two underscores or beginning with an
>> underscore and an upper case letter is at risk of colliding not only
>> with a particular library implementation (which we don't use), but
>> also of such with a particular compiler implementation (which we cannot
>> avoid to use). How can we permit use of such potentially problematic
>> identifiers?
> 
> Alternative question: is there a way we can check if there is clash of
> some sort between a compiler implementation of something and a MACRO or
> identifier we have in Xen? An error or a warning from the compiler for
> instance? That could be an easy way to prove we are safe.

Well. I think it is the default for the compiler to warn when re-#define-
ing a previously #define-d (by the compiler or by us) symbol, so on that
side we ought to be safe at any given point in time, yet we're still
latently unsafe (as to compilers introducing new pre-defines). For
built-in declarations, though, there's nothing I'm aware of that would
indicate collisions.

> Also, can we use the fact that the compiler we use is the same compiler
> used to compile Linux, and Linux makes extensive use of identifiers and
> macros starting with underscores as one of the reason for being safe
> from clashes?

I think we could, but I don't think we should.

Jan

Re: [PATCH 1/2] automation/eclair_analysis: deviate MISRA C Rule 21.2

[Stefano Stabellini]

On Fri, 21 Jun 2024, Jan Beulich wrote:
> On 21.06.2024 03:02, Stefano Stabellini wrote:
> > On Thu, 20 Jun 2024, Jan Beulich wrote:
> >> On 19.06.2024 19:09, Alessandro Zucchelli wrote:
> >>> Rule 21.2 reports identifiers reserved for the C and POSIX standard
> >>> libraries: all xen's translation units are compiled with option
> >>> -nostdinc, this guarantees that these libraries are not used, therefore
> >>> a justification is provided for allowing uses of such identifiers in
> >>> the project.
> >>> Builtins starting with "__builtin_" still remain available.
> >>>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> >>> ---
> >>>  automation/eclair_analysis/ECLAIR/deviations.ecl | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> index 447c1e6661..9fa9a7f01c 100644
> >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> @@ -487,6 +487,17 @@ leads to a violation of the Rule are deviated."
> >>>  # Series 21.
> >>>  #
> >>>  
> >>> +-doc_begin="Rules 21.1 and 21.2 report identifiers reserved for the C and POSIX
> >>> +standard libraries: if these libraries are not used there is no reason to avoid such
> >>> +identifiers. All xen's translation units are compiled with option -nostdinc,
> >>> +this guarantees that these libraries are not used. Some compilers could perform
> >>> +optimization using built-in functions: this risk is partially addressed by
> >>> +using the compilation option -fno-builtin. Builtins starting with \"__builtin_\"
> >>> +still remain available."
> >>
> >> While the sub-section "Reserved Identifiers" is part of Section 7,
> >> "Library", close coordination is needed between the library and the
> >> compiler, which only together form an "implementation". Therefore any
> >> use of identifiers beginning with two underscores or beginning with an
> >> underscore and an upper case letter is at risk of colliding not only
> >> with a particular library implementation (which we don't use), but
> >> also of such with a particular compiler implementation (which we cannot
> >> avoid to use). How can we permit use of such potentially problematic
> >> identifiers?
> > 
> > Alternative question: is there a way we can check if there is clash of
> > some sort between a compiler implementation of something and a MACRO or
> > identifier we have in Xen? An error or a warning from the compiler for
> > instance? That could be an easy way to prove we are safe.
> 
> Well. I think it is the default for the compiler to warn when re-#define-
> ing a previously #define-d (by the compiler or by us) symbol, so on that
> side we ought to be safe at any given point in time,

OK, that's good. It seems to me that this explanation should be part of
the deviation text.


> yet we're still latently unsafe (as to compilers introducing new
> pre-defines).

Sure, but we don't need to be safe in relation to future compiler. Right
now, we are targeting gcc-12.1.0 as written in
docs/misra/C-language-toolchain.rst. When we decide to enable a new
compiler in Xen we can fix/change any specific define as needed. Also
note the large amount of things written in C-language-toolchain.rst that
need to be checked and verified for a new compiler to make sure we can
actually use it safely (we make many assumptions).


> For built-in declarations, though, there's nothing I'm aware of that
> would indicate collisions.

For builtins, Alessandro was suggesting -fno-builtin. One question to
Alessandro is why would -fno-builtin only "partially" address the
problem.

Another question for Jan and also Alessandro: given that builtins
starting with __builtin_ remain available, any drawbacks in using
-fno-builtin in a Xen build?



> > Also, can we use the fact that the compiler we use is the same compiler
> > used to compile Linux, and Linux makes extensive use of identifiers and
> > macros starting with underscores as one of the reason for being safe
> > from clashes?
> 
> I think we could, but I don't think we should.

Re: [PATCH 1/2] automation/eclair_analysis: deviate MISRA C Rule 21.2

[Jan Beulich]

On 22.06.2024 01:27, Stefano Stabellini wrote:
> On Fri, 21 Jun 2024, Jan Beulich wrote:
>> On 21.06.2024 03:02, Stefano Stabellini wrote:
>>> On Thu, 20 Jun 2024, Jan Beulich wrote:
>>>> On 19.06.2024 19:09, Alessandro Zucchelli wrote:
>>>>> Rule 21.2 reports identifiers reserved for the C and POSIX standard
>>>>> libraries: all xen's translation units are compiled with option
>>>>> -nostdinc, this guarantees that these libraries are not used, therefore
>>>>> a justification is provided for allowing uses of such identifiers in
>>>>> the project.
>>>>> Builtins starting with "__builtin_" still remain available.
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
>>>>> ---
>>>>>  automation/eclair_analysis/ECLAIR/deviations.ecl | 11 +++++++++++
>>>>>  1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> index 447c1e6661..9fa9a7f01c 100644
>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> @@ -487,6 +487,17 @@ leads to a violation of the Rule are deviated."
>>>>>  # Series 21.
>>>>>  #
>>>>>  
>>>>> +-doc_begin="Rules 21.1 and 21.2 report identifiers reserved for the C and POSIX
>>>>> +standard libraries: if these libraries are not used there is no reason to avoid such
>>>>> +identifiers. All xen's translation units are compiled with option -nostdinc,
>>>>> +this guarantees that these libraries are not used. Some compilers could perform
>>>>> +optimization using built-in functions: this risk is partially addressed by
>>>>> +using the compilation option -fno-builtin. Builtins starting with \"__builtin_\"
>>>>> +still remain available."
>>>>
>>>> While the sub-section "Reserved Identifiers" is part of Section 7,
>>>> "Library", close coordination is needed between the library and the
>>>> compiler, which only together form an "implementation". Therefore any
>>>> use of identifiers beginning with two underscores or beginning with an
>>>> underscore and an upper case letter is at risk of colliding not only
>>>> with a particular library implementation (which we don't use), but
>>>> also of such with a particular compiler implementation (which we cannot
>>>> avoid to use). How can we permit use of such potentially problematic
>>>> identifiers?
>>>
>>> Alternative question: is there a way we can check if there is clash of
>>> some sort between a compiler implementation of something and a MACRO or
>>> identifier we have in Xen? An error or a warning from the compiler for
>>> instance? That could be an easy way to prove we are safe.
>>
>> Well. I think it is the default for the compiler to warn when re-#define-
>> ing a previously #define-d (by the compiler or by us) symbol, so on that
>> side we ought to be safe at any given point in time,
> 
> OK, that's good. It seems to me that this explanation should be part of
> the deviation text.
> 
> 
>> yet we're still latently unsafe (as to compilers introducing new
>> pre-defines).
> 
> Sure, but we don't need to be safe in relation to future compiler. Right
> now, we are targeting gcc-12.1.0 as written in
> docs/misra/C-language-toolchain.rst. When we decide to enable a new
> compiler in Xen we can fix/change any specific define as needed. Also
> note the large amount of things written in C-language-toolchain.rst that
> need to be checked and verified for a new compiler to make sure we can
> actually use it safely (we make many assumptions).
> 
> 
>> For built-in declarations, though, there's nothing I'm aware of that
>> would indicate collisions.
> 
> For builtins, Alessandro was suggesting -fno-builtin. One question to
> Alessandro is why would -fno-builtin only "partially" address the
> problem.
> 
> Another question for Jan and also Alessandro: given that builtins
> starting with __builtin_ remain available, any drawbacks in using
> -fno-builtin in a Xen build?

Just to mention it - we're building with -fno-builtin already anyway.
What I'm puzzled by is that it looks like I was under the wrong
impression that we're actually building -ffreestanding (which implies
-fno-builtin).

Jan

[PATCH 2/2] x86/APIC: address violation of MISRA C Rule 21.2

[Alessandro Zucchelli]

From: Nicola Vetrini <nicola.vetrini@bugseng.com>

The rule disallows the usage of an identifier reserved by the C standard.
All identfiers starting with '__' are reserved for any use, so the label
can be renamed in order to avoid the violation.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 6567af685a..2a60e6fe26 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -925,7 +925,7 @@ void __init init_apic_mappings(void)
     unsigned long apic_phys;
 
     if ( x2apic_enabled )
-        goto __next;
+        goto next;
     /*
      * If no local APIC can be found then set up a fake all
      * zeroes page to simulate the local APIC and another
@@ -941,7 +941,7 @@ void __init init_apic_mappings(void)
     apic_printk(APIC_VERBOSE, "mapped APIC to %08Lx (%08lx)\n", APIC_BASE,
                 apic_phys);
 
-__next:
+next:
     /*
      * Fetch the APIC ID of the BSP in case we have a
      * default configuration (or the MP table is broken).
-- 
2.34.1

Re: [PATCH 2/2] x86/APIC: address violation of MISRA C Rule 21.2

[Jan Beulich]

On 19.06.2024 19:09, Alessandro Zucchelli wrote:
> From: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> The rule disallows the usage of an identifier reserved by the C standard.
> All identfiers starting with '__' are reserved for any use, so the label
> can be renamed in order to avoid the violation.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

While the code change is certainly okay (with a cosmetic remark below),
you sending a change Nicola made requires, aiui, you own S-o-b as well.

> @@ -941,7 +941,7 @@ void __init init_apic_mappings(void)
>      apic_printk(APIC_VERBOSE, "mapped APIC to %08Lx (%08lx)\n", APIC_BASE,
>                  apic_phys);
>  
> -__next:
> +next:

While touching this (or any) label, can you please also make sure that
from now on it respects this section of ./CODING_STYLE (part of the
section "Indentation")?

"Due to the behavior of GNU diffutils "diff -p", labels should be
 indented by at least one blank.  Non-case labels inside switch() bodies
 are preferred to be indented the same as the block's case labels."

Jan