Re: [XEN PATCH v2 00/10] address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 29 Sep 2023, Simone Ballarin wrote:
> On 29/09/23 00:24, Stefano Stabellini wrote:
> > On Thu, 28 Sep 2023, Jan Beulich wrote:
> > > On 28.09.2023 15:17, Simone Ballarin wrote:
> > > > On 28/09/23 14:51, Jan Beulich wrote:
> > > > > On 28.09.2023 14:46, Simone Ballarin wrote:
> > > > > > On 13/09/23 10:02, Jan Beulich wrote:
> > > > > > > On 12.09.2023 11:36, Simone Ballarin wrote:
> > > > > > > > Add or move inclusion guards to address violations of
> > > > > > > > MISRA C:2012 Directive 4.10 ("Precautions shall be taken in
> > > > > > > > order
> > > > > > > > to prevent the contents of a header file being included more
> > > > > > > > than
> > > > > > > > once").
> > > > > > > > 
> > > > > > > > Inclusion guards must appear at the beginning of the headers
> > > > > > > > (comments are permitted anywhere) and the #if directive cannot
> > > > > > > > be used for other checks.
> > > > > > > > 
> > > > > > > > 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
> > > > > > > 
> > > > > > > Just to mention it here again for the entire series, seeing that
> > > > > > > despite
> > > > > > > my earlier comments to this effect a few R-b have arrived: If
> > > > > > > private
> > > > > > > headers need to gain guards (for, imo, no real reason), we first
> > > > > > > need to
> > > > > > > settle on a naming scheme for these guards, such that guards used
> > > > > > > in
> > > > > > > private headers aren't at risk of colliding with ones used headers
> > > > > > > living in one of the usual include directories. IOW imo fair parts
> > > > > > > of
> > > > > > > this series may need redoing.
> > > > > > > 
> > > > > > > Jan
> > > > > > > 
> > > > > > 
> > > > > > My proposal is:
> > > > > > - the relative path from "xen/arch" for files in this directory
> > > > > >      (i.e. X86_X86_X86_MMCONFIG_H for
> > > > > > "xen/arch/x86/x86_64/mmconfig.h";
> > > > > 
> > > > > X86_X86_64_MMCONFIG_H that is?
> > > > > 
> > > > > Yet then this scheme won't hold for xen/arch/include/asm/... ? It's
> > > > > also
> > > > > not clear whether you're deliberately omitting leading/trailing
> > > > > underscores
> > > > > here, which may be a way to distinguish private from global headers.
> > > > 
> > > > Each name that begins with a double or single underscore (__, _)
> > > > followed by an uppercase letter is reserved. Using a reserved identifier
> > > > is an undefined-b.
> > > > 
> > > > I would be better to avoid them.
> > > 
> > > I'm with you about avoiding them, except that we use such all over the
> > > place. Taking this together with ...
> > > 
> > > > > > - for the others, the entire path.
> > > > > 
> > > > > What exactly is "entire" here?
> > > > 
> > > > Let me try again.
> > > > 
> > > > If we are inside xen/arch the relative path starting from this
> > > > directory:
> > > >     | xen/arch/x86/include/asm/compat.h
> > > >     X86_INCLUDE_ASM_COMPAT_H
> > > > 
> > > > For xen/include, the current convention.
> > > > Maybe, in a future patch, we can consider removing the leading _.
> > > > 
> > > > For the others, the relative path after xen:
> > > >     | xen/common/efi/efi.h
> > > >     COMMON_EFI_EFI_H
> > > 
> > > ... this you're effectively suggesting to change all existing guards.
> > > That's an option, but likely not a preferred one. Personally I'd prefer
> > > if in particular the headers in xen/include/ and in xen/arch/*include/
> > > didn't needlessly include _INCLUDE_ in their guard names.
> > > 
> > > I'm really curious what others think.
> > 
> > If it is a MISRA requirement to avoid names that begin with single or
> > double underscore, then I think we should bite the bullet and change all
> > guard names, taking the opportunity to make them consistent.
> > 
> 
> Yes, it is.
> 
> Rule 21.2, found in Set3, addresses this Undef.-b.:
> A reserved identifier or reserved macro name shall not be declared.

OK. Adding Roberto in CC. I think we should discuss 21.2 during the next
MISRA C meeting. If 21.2 is accepted then we should go down the route of
a global rename here which would also benefit consistency across the
codebase.


> > If it is not a MISRA requirement, then I think we should go for the path
> > of least resistance and try to make the smallest amount of changes
> > overall, which seems to be:
> > 
> > - for xen/include/blah.h, __BLAH_H__
> > - for xen/arch/arm/asm/include/blah.h, __ASM_ARM_BLAH_H__
> > - for xen/arch/x86/asm/include/blah.h, it is far less consistent, maybe
> > __ASM_X86_BLAH_H__ ?