From: Stefano Stabellini <stefano.stabellini@amd.com>

For Dir 1.1, a document describing all implementation-defined behaviour
(i.e. gcc-specific behavior) will be added to docs/misra, also including
implementation-specific (gcc-specific) appropriate types for bit-field
relevant to Rule 6.1.

Rule 21.21 is lacking an example on gitlab but the rule is
straightforward: we don't use stdlib at all in Xen.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
We also discussed Rules 2.1, 5.5 and 7.4 but they require more work
before we can decide one way or the other.
---
 docs/misra/rules.rst | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index d5a6ee8cb6..0e298edeb8 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -40,6 +40,12 @@ existing codebase are work-in-progress.
      - Summary
      - Notes
 
+   * - `Dir 1.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_01_01.c>`_
+     - Required
+     - Any implementation-defined behaviour on which the output of the
+       program depends shall be documented and understood
+     -
+
    * - `Dir 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_02_01.c>`_
      - Required
      - All source files shall compile without any compilation errors
@@ -57,6 +63,13 @@ existing codebase are work-in-progress.
        header file being included more than once
      -
 
+   * - `Dir 4.11 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_11.c>`_
+     - Required
+     - The validity of values passed to library functions shall be checked
+     - We do not have libraries in Xen (libfdt and others are not
+       considered libraries from MISRA C point of view as they are
+       imported in source form)
+
    * - `Dir 4.14 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_14.c>`_
      - Required
      - The validity of values received from external sources shall be
@@ -133,6 +146,16 @@ existing codebase are work-in-progress.
        headers (xen/include/public/) are allowed to retain longer
        identifiers for backward compatibility.
 
+   * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
+     - Required
+     - A typedef name shall be a unique identifier
+     -
+
+   * - `Rule 6.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_06_01.c>`_
+     - Required
+     - Bit-fields shall only be declared with an appropriate type
+     -
+
    * - `Rule 6.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_06_02.c>`_
      - Required
      - Single-bit named bit fields shall not be of a signed type
@@ -143,6 +166,12 @@ existing codebase are work-in-progress.
      - Octal constants shall not be used
      -
 
+   * - `Rule 7.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_02.c>`_
+     - Required
+     - A "u" or "U" suffix shall be applied to all integer constants
+       that are represented in an unsigned type
+     -
+
    * - `Rule 7.3 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_03.c>`_
      - Required
      - The lowercase character l shall not be used in a literal suffix
@@ -314,6 +343,11 @@ existing codebase are work-in-progress.
        used following a subsequent call to the same function
      -
 
+   * - Rule 21.21
+     - Required
+     - The Standard Library function system of <stdlib.h> shall not be used
+     -
+
    * - `Rule 22.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_22_02.c>`_
      - Mandatory
      - A block of memory shall only be freed if it was allocated by means of a
-- 
2.25.1

On 07.06.2023 03:38, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> For Dir 1.1, a document describing all implementation-defined behaviour
> (i.e. gcc-specific behavior) will be added to docs/misra, also including
> implementation-specific (gcc-specific) appropriate types for bit-field
> relevant to Rule 6.1.
> 
> Rule 21.21 is lacking an example on gitlab but the rule is
> straightforward: we don't use stdlib at all in Xen.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
> We also discussed Rules 2.1, 5.5 and 7.4 but they require more work
> before we can decide one way or the other.

Since I wasn't able to attend yesterday's meeting, please forgive a
couple of remarks:

> @@ -133,6 +146,16 @@ existing codebase are work-in-progress.
>         headers (xen/include/public/) are allowed to retain longer
>         identifiers for backward compatibility.
>  
> +   * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
> +     - Required
> +     - A typedef name shall be a unique identifier
> +     -

Considering that the rule requires uniqueness across the entire code
base (and hence precludes e.g. two functions having identically named
local typedefs), I'm a little puzzled this was adopted. I for one
question that a use like the one mentioned is really at risk of being
confusing. Instead I think that the need to change at least one of
the names is at risk of making the code less readable then, even if
ever so slightly. (All of this said - I don't know if we have any
violations of this rule.)

> +   * - `Rule 6.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_06_01.c>`_
> +     - Required
> +     - Bit-fields shall only be declared with an appropriate type
> +     -

This requires you have settled on what "an appropriate type" is, i.e.
whether our uses of e.g. types wider than int is meant to become a
deviation, or will need eliminating. I suppose the outcome of this
could do with mentioning as a remark here.

> @@ -143,6 +166,12 @@ existing codebase are work-in-progress.
>       - Octal constants shall not be used
>       -
>  
> +   * - `Rule 7.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_02.c>`_
> +     - Required
> +     - A "u" or "U" suffix shall be applied to all integer constants
> +       that are represented in an unsigned type
> +     -

"Represented in an unsigned type" means there is a dependency on the
target architecture and compiler capabilities: Representation can only
be determined once knowing the underlying binary ABI, and uses in #if
and alike require knowing the widest integer type's size that the
compiler supports. As a consequence this looks like it may require, in
certain cases, to add u/U conditionally. Any such conditionals pose a
risk of cluttering the code.

> @@ -314,6 +343,11 @@ existing codebase are work-in-progress.
>         used following a subsequent call to the same function
>       -
>  
> +   * - Rule 21.21
> +     - Required
> +     - The Standard Library function system of <stdlib.h> shall not be used
> +     -

I wish inapplicable (to us) rules would also be marked as such.

Jan

On Wed, 7 Jun 2023, Jan Beulich wrote:
> On 07.06.2023 03:38, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@amd.com>
> > 
> > For Dir 1.1, a document describing all implementation-defined behaviour
> > (i.e. gcc-specific behavior) will be added to docs/misra, also including
> > implementation-specific (gcc-specific) appropriate types for bit-field
> > relevant to Rule 6.1.
> > 
> > Rule 21.21 is lacking an example on gitlab but the rule is
> > straightforward: we don't use stdlib at all in Xen.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> > We also discussed Rules 2.1, 5.5 and 7.4 but they require more work
> > before we can decide one way or the other.
> 
> Since I wasn't able to attend yesterday's meeting, please forgive a
> couple of remarks:

No problem


> > @@ -133,6 +146,16 @@ existing codebase are work-in-progress.
> >         headers (xen/include/public/) are allowed to retain longer
> >         identifiers for backward compatibility.
> >  
> > +   * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
> > +     - Required
> > +     - A typedef name shall be a unique identifier
> > +     -
> 
> Considering that the rule requires uniqueness across the entire code
> base (and hence precludes e.g. two functions having identically named
> local typedefs), I'm a little puzzled this was adopted. I for one
> question that a use like the one mentioned is really at risk of being
> confusing. Instead I think that the need to change at least one of
> the names is at risk of making the code less readable then, even if
> ever so slightly. (All of this said - I don't know if we have any
> violations of this rule.)

I don't think we have many local typedefs and I think we have only few
violations if I remember right. I'll let Roberto confirm how many. This
rule was considered not a difficult rule (some difficult rules were
found, namely 2.1, 5.5 and 7.4.)


> > +   * - `Rule 6.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_06_01.c>`_
> > +     - Required
> > +     - Bit-fields shall only be declared with an appropriate type
> > +     -
> 
> This requires you have settled on what "an appropriate type" is, i.e.
> whether our uses of e.g. types wider than int is meant to become a
> deviation, or will need eliminating. I suppose the outcome of this
> could do with mentioning as a remark here.

Yes, Roberto showed the "appropriate types" for gcc and there were
plenty including unsigned long if I remember right. I didn't write the
full list down.

Roberto do you have the list ready? I can add it in the Notes section
here.


> > @@ -143,6 +166,12 @@ existing codebase are work-in-progress.
> >       - Octal constants shall not be used
> >       -
> >  
> > +   * - `Rule 7.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_02.c>`_
> > +     - Required
> > +     - A "u" or "U" suffix shall be applied to all integer constants
> > +       that are represented in an unsigned type
> > +     -
> 
> "Represented in an unsigned type" means there is a dependency on the
> target architecture and compiler capabilities: Representation can only
> be determined once knowing the underlying binary ABI, and uses in #if
> and alike require knowing the widest integer type's size that the
> compiler supports. As a consequence this looks like it may require, in
> certain cases, to add u/U conditionally. Any such conditionals pose a
> risk of cluttering the code.

I don't think there is any plan to add u/U conditionally, and I can see
that would be undesirable. I think the idea is to add u/U to all
constants meant to be unsigned. But also here I'll Roberto chime in --
he said they already have a draft patch to fix this.


> > @@ -314,6 +343,11 @@ existing codebase are work-in-progress.
> >         used following a subsequent call to the same function
> >       -
> >  
> > +   * - Rule 21.21
> > +     - Required
> > +     - The Standard Library function system of <stdlib.h> shall not be used
> > +     -
> 
> I wish inapplicable (to us) rules would also be marked as such.
 
Yes, actually it could be a good idea to say "inapplicable" in the Notes
section for all the rules like this one. I can write a patch for it.

Hi there.

Please see below.

On 07/06/23 23:53, Stefano Stabellini wrote:
> On Wed, 7 Jun 2023, Jan Beulich wrote:
>>> +   * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
>>> +     - Required
>>> +     - A typedef name shall be a unique identifier
>>> +     -
>>
>> Considering that the rule requires uniqueness across the entire code
>> base (and hence precludes e.g. two functions having identically named
>> local typedefs), I'm a little puzzled this was adopted. I for one
>> question that a use like the one mentioned is really at risk of being
>> confusing. Instead I think that the need to change at least one of
>> the names is at risk of making the code less readable then, even if
>> ever so slightly. (All of this said - I don't know if we have any
>> violations of this rule.)
> 
> I don't think we have many local typedefs and I think we have only few
> violations if I remember right. I'll let Roberto confirm how many. This
> rule was considered not a difficult rule (some difficult rules were
> found, namely 2.1, 5.5 and 7.4.)

There currently are 30 violations for ARM64:

- some involve a typedef module_name (in the call it was said this should
   be renamed module_name_t, which will solve the issue);
- most concern repeated typedefs (UINT64 and similar) which are repeated
   in xen/arch/arm/include/asm/arm64/efibind.h
   and xen/include/acpi/actypes.h
- ret_t and phys_addr_t are also redefined in a couple of places.

There are 90 violations for X86_64, for the same reasons, plus

- another set of typedefs for basic types (such as u8);
- repeated typedefs for things like guest_l1e_t in the same header file:

xen/arch/x86/include/asm/guest_pt.h:60.39-60.49:
for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
xen/arch/x86/include/asm/guest_pt.h:128.22-128.32:
for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused

The indicated lines read as follows:

typedef struct { guest_intpte_t l1; } guest_l1e_t;
typedef l1_pgentry_t guest_l1e_t;

>>> +   * - `Rule 6.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_06_01.c>`_
>>> +     - Required
>>> +     - Bit-fields shall only be declared with an appropriate type
>>> +     -
>>
>> This requires you have settled on what "an appropriate type" is, i.e.
>> whether our uses of e.g. types wider than int is meant to become a
>> deviation, or will need eliminating. I suppose the outcome of this
>> could do with mentioning as a remark here.
> 
> Yes, Roberto showed the "appropriate types" for gcc and there were
> plenty including unsigned long if I remember right. I didn't write the
> full list down.
> 
> Roberto do you have the list ready? I can add it in the Notes section
> here.

GCC supports all integer types including enums.  In the analyzed
configurations of the project, bit-fields are declared of the
following types, in addition to the ones the compiler *has* to
support according to C99:

ARM64:   unsigned char, unsigned short, unsigned long, unsigned long long;
X86_64:  unsigned char, unsigned short, unsigned long, enum.

>>> @@ -143,6 +166,12 @@ existing codebase are work-in-progress.
>>>        - Octal constants shall not be used
>>>        -
>>>   
>>> +   * - `Rule 7.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_02.c>`_
>>> +     - Required
>>> +     - A "u" or "U" suffix shall be applied to all integer constants
>>> +       that are represented in an unsigned type
>>> +     -
>>
>> "Represented in an unsigned type" means there is a dependency on the
>> target architecture and compiler capabilities: Representation can only
>> be determined once knowing the underlying binary ABI, and uses in #if
>> and alike require knowing the widest integer type's size that the
>> compiler supports. As a consequence this looks like it may require, in
>> certain cases, to add u/U conditionally. Any such conditionals pose a
>> risk of cluttering the code.
> 
> I don't think there is any plan to add u/U conditionally, and I can see
> that would be undesirable. I think the idea is to add u/U to all
> constants meant to be unsigned. But also here I'll Roberto chime in --
> he said they already have a draft patch to fix this.

Yes, the patch will add U to all implicitly unsigned literals.
An open thing is whether it should also add that suffix in order
to avoid inconsistencies.  Here is an example:

/* INIT Record (for IPF) */
#define CPER_NOTIFY_INIT                                                \
         UUID_LE(0xCC5263E8, 0x9308, 0x454a, 0x89, 0xD0, 0x34, 0x0B,     \
                 0xD3, 0x9B, 0xC9, 0x8E)
/* Non-Maskable Interrupt */
#define CPER_NOTIFY_NMI                                                 \
         UUID_LE(0x5BAD89FF, 0xB7E6, 0x42c9, 0x81, 0x4A, 0xCF, 0x24,     \
                 0x85, 0xD6, 0xE9, 0x8A)

While 0xCC5263E8 is implicitly unsigned, 0x5BAD89FF is signed.
My inclination would be to add a U suffix to both, in order to
restore consistency in addition of complying with Rule 7.2.
Someone might say "I want to minimize the number of U suffixes
that are added, and keep the inconsistency".
Please note that the semantic inconsistency is present in the
original code;  adding one U would keep that semantic inconsistency
and introduce a visual inconsistency;  adding two Us would remove
both the semantic inconsistency and the visual inconsistency.
Given that the first argument of UUID_LE is used for lots of
bitwise operations, the option of adding two Us will also solve lots
of violations of Series 10 guidelines.

>>> @@ -314,6 +343,11 @@ existing codebase are work-in-progress.
>>>          used following a subsequent call to the same function
>>>        -
>>>   
>>> +   * - Rule 21.21
>>> +     - Required
>>> +     - The Standard Library function system of <stdlib.h> shall not be used
>>> +     -
>>
>> I wish inapplicable (to us) rules would also be marked as such.
>   
> Yes, actually it could be a good idea to say "inapplicable" in the Notes
> section for all the rules like this one. I can write a patch for it.

It depends on the audience: if you want to facilitate certification for Xen-based
products, "inapplicable" would not help, as no assessor would accept that.
What an assessor would want to see is "enforced, zero violations".

If, instead, your are writing for a different audience then "inapplicable"
can be appropriate, even though I think what you really mean is "any patch
introducing a call to stdlib.h's int system(const char *command) would
immediately be rejected by maintainers".  Which is not in contrast with
having the rule in the coding standard and having it automatically checked
by tools.

On 08.06.2023 13:02, Roberto Bagnara wrote:
> On 07/06/23 23:53, Stefano Stabellini wrote:
>> On Wed, 7 Jun 2023, Jan Beulich wrote:
>>>> +   * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
>>>> +     - Required
>>>> +     - A typedef name shall be a unique identifier
>>>> +     -
>>>
>>> Considering that the rule requires uniqueness across the entire code
>>> base (and hence precludes e.g. two functions having identically named
>>> local typedefs), I'm a little puzzled this was adopted. I for one
>>> question that a use like the one mentioned is really at risk of being
>>> confusing. Instead I think that the need to change at least one of
>>> the names is at risk of making the code less readable then, even if
>>> ever so slightly. (All of this said - I don't know if we have any
>>> violations of this rule.)
>>
>> I don't think we have many local typedefs and I think we have only few
>> violations if I remember right. I'll let Roberto confirm how many. This
>> rule was considered not a difficult rule (some difficult rules were
>> found, namely 2.1, 5.5 and 7.4.)
> 
> There currently are 30 violations for ARM64:
> 
> - some involve a typedef module_name (in the call it was said this should
>    be renamed module_name_t, which will solve the issue);
> - most concern repeated typedefs (UINT64 and similar) which are repeated
>    in xen/arch/arm/include/asm/arm64/efibind.h
>    and xen/include/acpi/actypes.h
> - ret_t and phys_addr_t are also redefined in a couple of places.
> 
> There are 90 violations for X86_64, for the same reasons, plus
> 
> - another set of typedefs for basic types (such as u8);
> - repeated typedefs for things like guest_l1e_t in the same header file:
> 
> xen/arch/x86/include/asm/guest_pt.h:60.39-60.49:
> for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
> xen/arch/x86/include/asm/guest_pt.h:128.22-128.32:
> for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
> 
> The indicated lines read as follows:
> 
> typedef struct { guest_intpte_t l1; } guest_l1e_t;
> typedef l1_pgentry_t guest_l1e_t;

So this is an example where I don't think we can sensibly do away with the
re-use of the same typedef name: We use it so we can build the same source
files multiple ways, dealing with different paging modes guests may be in.

Jan

On 09/06/23 10:46, Jan Beulich wrote:
> On 08.06.2023 13:02, Roberto Bagnara wrote:
>> On 07/06/23 23:53, Stefano Stabellini wrote:
>>> On Wed, 7 Jun 2023, Jan Beulich wrote:
>>>>> +   * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
>>>>> +     - Required
>>>>> +     - A typedef name shall be a unique identifier
>>>>> +     -
>>>>
>>>> Considering that the rule requires uniqueness across the entire code
>>>> base (and hence precludes e.g. two functions having identically named
>>>> local typedefs), I'm a little puzzled this was adopted. I for one
>>>> question that a use like the one mentioned is really at risk of being
>>>> confusing. Instead I think that the need to change at least one of
>>>> the names is at risk of making the code less readable then, even if
>>>> ever so slightly. (All of this said - I don't know if we have any
>>>> violations of this rule.)
>>>
>>> I don't think we have many local typedefs and I think we have only few
>>> violations if I remember right. I'll let Roberto confirm how many. This
>>> rule was considered not a difficult rule (some difficult rules were
>>> found, namely 2.1, 5.5 and 7.4.)
>>
>> There currently are 30 violations for ARM64:
>>
>> - some involve a typedef module_name (in the call it was said this should
>>     be renamed module_name_t, which will solve the issue);
>> - most concern repeated typedefs (UINT64 and similar) which are repeated
>>     in xen/arch/arm/include/asm/arm64/efibind.h
>>     and xen/include/acpi/actypes.h
>> - ret_t and phys_addr_t are also redefined in a couple of places.
>>
>> There are 90 violations for X86_64, for the same reasons, plus
>>
>> - another set of typedefs for basic types (such as u8);
>> - repeated typedefs for things like guest_l1e_t in the same header file:
>>
>> xen/arch/x86/include/asm/guest_pt.h:60.39-60.49:
>> for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
>> xen/arch/x86/include/asm/guest_pt.h:128.22-128.32:
>> for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
>>
>> The indicated lines read as follows:
>>
>> typedef struct { guest_intpte_t l1; } guest_l1e_t;
>> typedef l1_pgentry_t guest_l1e_t;
> 
> So this is an example where I don't think we can sensibly do away with the
> re-use of the same typedef name: We use it so we can build the same source
> files multiple ways, dealing with different paging modes guests may be in.

Typedefs being used this way can be deviated with tool configuration.
Here is a list of candidates for that treatment:

guest_intpte_t
guest_l1e_t
guest_l2e_t
ret_

I am not sure about the latter.  Please let me know if this is what
you would prefer and possible additions to/removals from the above list.
Kind regards,

    Roberto

On 12.06.2023 10:58, Roberto Bagnara wrote:
> On 09/06/23 10:46, Jan Beulich wrote:
>> On 08.06.2023 13:02, Roberto Bagnara wrote:
>>> On 07/06/23 23:53, Stefano Stabellini wrote:
>>>> On Wed, 7 Jun 2023, Jan Beulich wrote:
>>>>>> +   * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
>>>>>> +     - Required
>>>>>> +     - A typedef name shall be a unique identifier
>>>>>> +     -
>>>>>
>>>>> Considering that the rule requires uniqueness across the entire code
>>>>> base (and hence precludes e.g. two functions having identically named
>>>>> local typedefs), I'm a little puzzled this was adopted. I for one
>>>>> question that a use like the one mentioned is really at risk of being
>>>>> confusing. Instead I think that the need to change at least one of
>>>>> the names is at risk of making the code less readable then, even if
>>>>> ever so slightly. (All of this said - I don't know if we have any
>>>>> violations of this rule.)
>>>>
>>>> I don't think we have many local typedefs and I think we have only few
>>>> violations if I remember right. I'll let Roberto confirm how many. This
>>>> rule was considered not a difficult rule (some difficult rules were
>>>> found, namely 2.1, 5.5 and 7.4.)
>>>
>>> There currently are 30 violations for ARM64:
>>>
>>> - some involve a typedef module_name (in the call it was said this should
>>>     be renamed module_name_t, which will solve the issue);
>>> - most concern repeated typedefs (UINT64 and similar) which are repeated
>>>     in xen/arch/arm/include/asm/arm64/efibind.h
>>>     and xen/include/acpi/actypes.h
>>> - ret_t and phys_addr_t are also redefined in a couple of places.
>>>
>>> There are 90 violations for X86_64, for the same reasons, plus
>>>
>>> - another set of typedefs for basic types (such as u8);
>>> - repeated typedefs for things like guest_l1e_t in the same header file:
>>>
>>> xen/arch/x86/include/asm/guest_pt.h:60.39-60.49:
>>> for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
>>> xen/arch/x86/include/asm/guest_pt.h:128.22-128.32:
>>> for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
>>>
>>> The indicated lines read as follows:
>>>
>>> typedef struct { guest_intpte_t l1; } guest_l1e_t;
>>> typedef l1_pgentry_t guest_l1e_t;
>>
>> So this is an example where I don't think we can sensibly do away with the
>> re-use of the same typedef name: We use it so we can build the same source
>> files multiple ways, dealing with different paging modes guests may be in.
> 
> Typedefs being used this way can be deviated with tool configuration.
> Here is a list of candidates for that treatment:
> 
> guest_intpte_t
> guest_l1e_t
> guest_l2e_t
> ret_
> 
> I am not sure about the latter.  Please let me know if this is what
> you would prefer and possible additions to/removals from the above list.

Well, if deviating such is possible despite their extended use (in certain
places), then fine. I was afraid that a deviation with such wide a scope
might be hard to justify.

Jan

On Fri, 9 Jun 2023, Jan Beulich wrote:
> On 08.06.2023 13:02, Roberto Bagnara wrote:
> > On 07/06/23 23:53, Stefano Stabellini wrote:
> >> On Wed, 7 Jun 2023, Jan Beulich wrote:
> >>>> +   * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_
> >>>> +     - Required
> >>>> +     - A typedef name shall be a unique identifier
> >>>> +     -
> >>>
> >>> Considering that the rule requires uniqueness across the entire code
> >>> base (and hence precludes e.g. two functions having identically named
> >>> local typedefs), I'm a little puzzled this was adopted. I for one
> >>> question that a use like the one mentioned is really at risk of being
> >>> confusing. Instead I think that the need to change at least one of
> >>> the names is at risk of making the code less readable then, even if
> >>> ever so slightly. (All of this said - I don't know if we have any
> >>> violations of this rule.)
> >>
> >> I don't think we have many local typedefs and I think we have only few
> >> violations if I remember right. I'll let Roberto confirm how many. This
> >> rule was considered not a difficult rule (some difficult rules were
> >> found, namely 2.1, 5.5 and 7.4.)
> > 
> > There currently are 30 violations for ARM64:
> > 
> > - some involve a typedef module_name (in the call it was said this should
> >    be renamed module_name_t, which will solve the issue);
> > - most concern repeated typedefs (UINT64 and similar) which are repeated
> >    in xen/arch/arm/include/asm/arm64/efibind.h
> >    and xen/include/acpi/actypes.h
> > - ret_t and phys_addr_t are also redefined in a couple of places.
> > 
> > There are 90 violations for X86_64, for the same reasons, plus
> > 
> > - another set of typedefs for basic types (such as u8);
> > - repeated typedefs for things like guest_l1e_t in the same header file:
> > 
> > xen/arch/x86/include/asm/guest_pt.h:60.39-60.49:
> > for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
> > xen/arch/x86/include/asm/guest_pt.h:128.22-128.32:
> > for program `xen/.xen-syms.0', the identifier for typedef `guest_l1e_t' is reused
> > 
> > The indicated lines read as follows:
> > 
> > typedef struct { guest_intpte_t l1; } guest_l1e_t;
> > typedef l1_pgentry_t guest_l1e_t;
> 
> So this is an example where I don't think we can sensibly do away with the
> re-use of the same typedef name: We use it so we can build the same source
> files multiple ways, dealing with different paging modes guests may be in.

Maybe we can have a quick discussion on Rule 5.6 during the next
meeting.

In the meantime, I'll resend the patch without Rule 5.6 and additing the
extra types in the notes for 6.1.