mbox series

[XEN,v3,00/16] xen: address violation of MISRA C:2012 Directive 4.10

Message ID cover.1710145041.git.simone.ballarin@bugseng.com (mailing list archive)
Headers show
Series xen: address violation of MISRA C:2012 Directive 4.10 | expand

Message

Simone Ballarin March 11, 2024, 8:59 a.m. UTC
The Xen sources contain violations of MISRA C:2012 Directive 4.10 whose headline states:
"Precautions shall be taken in order to prevent the contents of a header file
being included more than once".

As stated in v2, the following naming convention has been estabilished:
- arch/.../include/asm/ headers -> ASM_<filename>_H
- private headers -> <dir>_<filename>_H
- asm-generic headers -> ASM_GENERIC_<filename>_H

Since there would have been conflicting guards between architectures (which is a violation
of the directive), there has been a need for ASM headers to specify  if the inclusion guard
referred to ARM or X86. Hence it has been decided to adopt instead:
- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H

The subdir used is the smallest possible to avoid collisions. For example, it has been
observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there
are plenty of header files with the same name, hence  _ARMxx_ was added as subdirectory.

There has been a need to define a standard for generated headers too:
 - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
 - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H

To summarize, here are all the rules that have been applied:
- private headers -> <dir>_<filename>_H
- asm-generic headers -> ASM_GENERIC_<filename>_H
- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
- include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
- arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H

Links to the discussions:
https://lists.xenproject.org/archives/html/xen-devel/2023-09/msg01928.html
https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg01784.html
https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg02073.html

Changes in v3:
Add/amend inclusion guards to address violations of the Directive and the new naming convention.
Remove trailing underscores.
Modify creation rule for asm-offsets.h to conform to the new standard and to not generate conflicting
guards between architectures (which is a violation of the Directive).

Maria Celeste Cesario (6):
  xen/arm: address violations of MISRA C:2012 Directive 4.10
  xen: address violations of MISRA C:2012 Directive 4.10
  xen: add deviations for MISRA C.2012 Directive 4.10
  xen/x86: address violations of MISRA C:2012 Directive 4.10
  x86/mtrr: address violations of MISRA C:2012 Directive 4.10
  xen/lz4: address violations of MISRA C:2012 Directive 4.10

Simone Ballarin (10):
  misra: add deviation for headers that explicitly avoid guards
  misra: modify deviations for empty and generated headers
  misra: add deviations for direct inclusion guards
  xen/arm: address violations of MISRA C:2012 Directive 4.10
  xen/x86: address violations of MISRA C:2012 Directive 4.10
  x86/EFI: address violations of MISRA C:2012 Directive 4.10
  xen/common: address violations of MISRA C:2012 Directive 4.10
  xen/efi: address violations of MISRA C:2012 Directive 4.10
  xen: address violations of MISRA C:2012 Directive 4.10
  x86/asm: address violations of MISRA C:2012 Directive 4.10

 .../eclair_analysis/ECLAIR/deviations.ecl     | 12 +++---
 docs/misra/deviations.rst                     |  7 ++++
 docs/misra/safe.json                          | 40 +++++++++++++++++++
 xen/arch/arm/efi/efi-boot.h                   |  6 +++
 xen/arch/arm/efi/runtime.h                    |  1 +
 xen/arch/arm/include/asm/domain.h             |  6 +--
 xen/arch/arm/include/asm/efibind.h            |  5 +++
 xen/arch/arm/include/asm/event.h              |  6 +--
 xen/arch/arm/include/asm/grant_table.h        |  6 +--
 xen/arch/arm/include/asm/hypercall.h          |  1 +
 xen/arch/arm/include/asm/io.h                 |  6 +--
 xen/arch/arm/include/asm/irq.h                |  6 +--
 xen/arch/arm/include/asm/smp.h                |  6 +--
 xen/arch/arm/include/asm/spinlock.h           |  6 +--
 xen/arch/arm/include/asm/system.h             |  6 +--
 xen/arch/x86/Makefile                         | 10 +++--
 xen/arch/x86/cpu/cpu.h                        |  5 +++
 xen/arch/x86/cpu/mtrr/mtrr.h                  |  4 ++
 xen/arch/x86/efi/efi-boot.h                   |  7 ++++
 xen/arch/x86/efi/runtime.h                    |  5 +++
 xen/arch/x86/include/asm/compat.h             |  5 +++
 xen/arch/x86/include/asm/cpufeatures.h        |  5 +--
 xen/arch/x86/include/asm/domain.h             |  6 +--
 xen/arch/x86/include/asm/efibind.h            |  5 +++
 xen/arch/x86/include/asm/event.h              |  6 +--
 xen/arch/x86/include/asm/grant_table.h        |  6 +--
 xen/arch/x86/include/asm/hypercall.h          |  1 +
 xen/arch/x86/include/asm/io.h                 |  6 +--
 xen/arch/x86/include/asm/irq.h                |  6 +--
 xen/arch/x86/include/asm/smp.h                |  6 +--
 xen/arch/x86/include/asm/spinlock.h           |  6 +--
 xen/arch/x86/include/asm/system.h             |  6 +--
 xen/arch/x86/x86_64/mmconfig.h                |  5 +++
 xen/arch/x86/x86_emulate/private.h            |  5 +++
 xen/build.mk                                  |  6 ++-
 xen/common/decompress.h                       |  5 +++
 xen/common/efi/efi.h                          |  5 +++
 xen/common/event_channel.h                    |  5 +++
 xen/common/lz4/defs.h                         |  5 +++
 xen/include/Makefile                          | 12 ++++--
 xen/include/public/arch-x86/cpufeatureset.h   |  1 +
 xen/include/public/arch-x86/xen.h             |  1 +
 xen/include/public/errno.h                    |  1 +
 xen/include/xen/err.h                         |  8 ++--
 xen/include/xen/pci_ids.h                     |  5 +++
 xen/include/xen/softirq.h                     |  8 ++--
 xen/include/xen/vmap.h                        |  8 ++--
 xen/scripts/Makefile.asm-generic              | 16 +++++++-
 48 files changed, 233 insertions(+), 78 deletions(-)

Comments

Jan Beulich March 11, 2024, 9:59 a.m. UTC | #1
On 11.03.2024 09:59, Simone Ballarin wrote:
> The Xen sources contain violations of MISRA C:2012 Directive 4.10 whose headline states:
> "Precautions shall be taken in order to prevent the contents of a header file
> being included more than once".
> 
> As stated in v2, the following naming convention has been estabilished:
> - arch/.../include/asm/ headers -> ASM_<filename>_H
> - private headers -> <dir>_<filename>_H
> - asm-generic headers -> ASM_GENERIC_<filename>_H
> 
> Since there would have been conflicting guards between architectures (which is a violation
> of the directive),

Why would this be a violation? The two sets of headers aren't use together,
and any scan should only point out collisions in what's actively used, I'd
hope.

> there has been a need for ASM headers to specify  if the inclusion guard
> referred to ARM or X86. Hence it has been decided to adopt instead:
> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> 
> The subdir used is the smallest possible to avoid collisions. For example, it has been
> observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there
> are plenty of header files with the same name, hence  _ARMxx_ was added as subdirectory.

Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get
away with one less name component.

> There has been a need to define a standard for generated headers too:
>  - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
>  - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
> 
> To summarize, here are all the rules that have been applied:
> - private headers -> <dir>_<filename>_H

And <dir> here is the full path? I thought I had indicated before that
this is going to lead to excessively long identifiers.

> - asm-generic headers -> ASM_GENERIC_<filename>_H
> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
> - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H

And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which
doesn't seem very likely)?

Jan
Simone Ballarin March 11, 2024, 11:41 a.m. UTC | #2
On 11/03/24 10:59, Jan Beulich wrote:
> On 11.03.2024 09:59, Simone Ballarin wrote:
>> The Xen sources contain violations of MISRA C:2012 Directive 4.10 whose headline states:
>> "Precautions shall be taken in order to prevent the contents of a header file
>> being included more than once".
>>
>> As stated in v2, the following naming convention has been estabilished:
>> - arch/.../include/asm/ headers -> ASM_<filename>_H
>> - private headers -> <dir>_<filename>_H
>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>
>> Since there would have been conflicting guards between architectures (which is a violation
>> of the directive),
> 
> Why would this be a violation? The two sets of headers aren't use together,
> and any scan should only point out collisions in what's actively used, I'd
> hope.
> 

A header guard cannot be used multiple times in the same project, so it's a question
of whether or not we want to consider each arch variant part of the same project.
In case, we can define a new rule for these files.

>> there has been a need for ASM headers to specify  if the inclusion guard
>> referred to ARM or X86. Hence it has been decided to adopt instead:
>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>
>> The subdir used is the smallest possible to avoid collisions. For example, it has been
>> observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there
>> are plenty of header files with the same name, hence  _ARMxx_ was added as subdirectory.
> 
> Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get
> away with one less name component.
> 

I agree.

>> There has been a need to define a standard for generated headers too:
>>   - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
>>   - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
>>
>> To summarize, here are all the rules that have been applied:
>> - private headers -> <dir>_<filename>_H
> 
> And <dir> here is the full path? I thought I had indicated before that
> this is going to lead to excessively long identifiers.
> 

Yes, dir is the full path. This general fallback rule to use when more specific rules do not apply.
If this generates excessively long guards, we can simply add more rules.
>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>> - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
>> - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
> 
> And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which
> doesn't seem very likely)?
> 

Yes, it is. Do you prefer dropping the _ARM_?

> Jan
Jan Beulich March 11, 2024, 1:27 p.m. UTC | #3
On 11.03.2024 12:41, Simone Ballarin wrote:
> On 11/03/24 10:59, Jan Beulich wrote:
>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>> The Xen sources contain violations of MISRA C:2012 Directive 4.10 whose headline states:
>>> "Precautions shall be taken in order to prevent the contents of a header file
>>> being included more than once".
>>>
>>> As stated in v2, the following naming convention has been estabilished:
>>> - arch/.../include/asm/ headers -> ASM_<filename>_H
>>> - private headers -> <dir>_<filename>_H
>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>>
>>> Since there would have been conflicting guards between architectures (which is a violation
>>> of the directive),
>>
>> Why would this be a violation? The two sets of headers aren't use together,
>> and any scan should only point out collisions in what's actively used, I'd
>> hope.
>>
> 
> A header guard cannot be used multiple times in the same project, so it's a question
> of whether or not we want to consider each arch variant part of the same project.
> In case, we can define a new rule for these files.
> 
>>> there has been a need for ASM headers to specify  if the inclusion guard
>>> referred to ARM or X86. Hence it has been decided to adopt instead:
>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>>
>>> The subdir used is the smallest possible to avoid collisions. For example, it has been
>>> observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there
>>> are plenty of header files with the same name, hence  _ARMxx_ was added as subdirectory.
>>
>> Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get
>> away with one less name component.
>>
> 
> I agree.
> 
>>> There has been a need to define a standard for generated headers too:
>>>   - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
>>>   - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
>>>
>>> To summarize, here are all the rules that have been applied:
>>> - private headers -> <dir>_<filename>_H
>>
>> And <dir> here is the full path? I thought I had indicated before that
>> this is going to lead to excessively long identifiers.
>>
> 
> Yes, dir is the full path. This general fallback rule to use when more specific rules do not apply.
> If this generates excessively long guards, we can simply add more rules.
>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>> - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
>>> - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
>>
>> And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which
>> doesn't seem very likely)?
> 
> Yes, it is. Do you prefer dropping the _ARM_?

Well, I prefer any measure keeping down the length of these identifiers.

Jan