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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 24 Oct 2023, Jan Beulich wrote:
> On 24.10.2023 15:31, Julien Grall wrote:
> > Hi Stefano,
> > 
> > On 23/10/2023 21:47, Stefano Stabellini wrote:
> >> On Mon, 23 Oct 2023, Jan Beulich wrote:
> >>> On 21.10.2023 01:26, Stefano Stabellini wrote:
> >>>> On Fri, 20 Oct 2023, Jan Beulich wrote:
> >>>>> On 19.10.2023 18:19, Stefano Stabellini wrote:
> >>>>>> On Thu, 19 Oct 2023, Jan Beulich wrote:
> >>>>>>> On 19.10.2023 02:44, Stefano Stabellini wrote:
> >>>>>>>> On Wed, 18 Oct 2023, Jan Beulich wrote:
> >>>>>>>>> On 18.10.2023 02:48, Stefano Stabellini wrote:
> >>>>>>>>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
> >>>>>>>>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
> >>>>>>>>>>>> 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:
> >>>>>>>>>>>
> >>>>>>>>>>> ... "least resistance" won't gain us much, as hardly any guards 
> >>>>>>>>>>> don't
> >>>>>>>>>>> start with an underscore.
> >>>>>>>>>>>
> >>>>>>>>>>>> - 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__ ?
> >>>>>>>>>>>
> >>>>>>>>>>> There are no headers in xen/include/. For (e.g.) xen/include/xen/ 
> >>>>>>>>>>> we
> >>>>>>>>>>> may go with XEN_BLAH_H; whether ASM prefixes are needed I'm not 
> >>>>>>>>>>> sure;
> >>>>>>>>>>> we could go with just ARM_BLAH_H and X86_BLAH_H?
> >>>>>>>>>>>
> >>>>>>>>>>> The primary question though is (imo) how to deal with private 
> >>>>>>>>>>> headers,
> >>>>>>>>>>> such that the risk of name collisions is as small as possible.
> >>>>>>>>>>
> >>>>>>>>>> Looking at concrete examples under xen/include/xen:
> >>>>>>>>>> xen/include/xen/mm.h __XEN_MM_H__
> >>>>>>>>>> xen/include/xen/dm.h __XEN_DM_H__
> >>>>>>>>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
> >>>>>>>>>>
> >>>>>>>>>> So I think we should do for consistency:
> >>>>>>>>>> xen/include/xen/blah.h __XEN_BLAH_H__
> >>>>>>>>>>
> >>>>>>>>>> Even if we know the leading underscore are undesirable, in this 
> >>>>>>>>>> case I
> >>>>>>>>>> would prefer consistency.
> >>>>>>>>>
> >>>>>>>>> I'm kind of okay with that. FTAOD - here and below you mean to make 
> >>>>>>>>> this
> >>>>>>>>> one explicit first exception from the "no new leading underscores" 
> >>>>>>>>> goal,
> >>>>>>>>> for newly added headers?
> >>>>>>>>
> >>>>>>>> Yes. The reason is for consistency with the existing header files.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>> On the other hand looking at ARM examples:
> >>>>>>>>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> >>>>>>>>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> >>>>>>>>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> >>>>>>>>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
> >>>>>>>>>>
> >>>>>>>>>> And also looking at x86 examples:
> >>>>>>>>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> >>>>>>>>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> >>>>>>>>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
> >>>>>>>>>>
> >>>>>>>>>> Thet are very inconsistent.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> So for ARM and X86 headers I think we are free to pick anything we 
> >>>>>>>>>> want,
> >>>>>>>>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine 
> >>>>>>>>>> by
> >>>>>>>>>> me.
> >>>>>>>>>
> >>>>>>>>> To be honest, I'd prefer a global underlying pattern, i.e. if common
> >>>>>>>>> headers are "fine" to use leading underscores for guards, arch 
> >>>>>>>>> header
> >>>>>>>>> should, too.
> >>>>>>>>
> >>>>>>>> I am OK with that too. We could go with:
> >>>>>>>> __ASM_ARM_BLAH_H__
> >>>>>>>> __ASM_X86_BLAH_H__
> >>>>>>>>
> >>>>>>>> I used "ASM" to make it easier to differentiate with the private 
> >>>>>>>> headers
> >>>>>>>> below. Also the version without "ASM" would work but it would only
> >>>>>>>> differ with the private headers in terms of leading underscores. I
> >>>>>>>> thought that also having "ASM" would help readability and help avoid
> >>>>>>>> confusion.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>> For private headers such as:
> >>>>>>>>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> >>>>>>>>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> >>>>>>>>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> >>>>>>>>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> >>>>>>>>>>
> >>>>>>>>>> More similar but still inconsistent. I would go with 
> >>>>>>>>>> ARCH_ARM_BLAH_H and
> >>>>>>>>>> ARCH_X86_BLAH_H for new headers.
> >>>>>>>>>
> >>>>>>>>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
> >>>>>>>>> guard names. If we continue to use double-underscore prefixed names
> >>>>>>>>> in common and arch headers, why don't we demand no leading 
> >>>>>>>>> underscores
> >>>>>>>>> and no path-derived prefixes in private headers? That'll avoid any
> >>>>>>>>> collisions between the two groups.
> >>>>>>>>
> >>>>>>>> OK, so for private headers:
> >>>>>>>>
> >>>>>>>> ARM_BLAH_H
> >>>>>>>> X86_BLAH_H
> >>>>>>>>
> >>>>>>>> What that work for you?
> >>>>>>>
> >>>>>>> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to 
> >>>>>>> ask
> >>>>>>> differently, how would you see e.g. common/decompress.h's guard named?
> >>>>>>
> >>>>>> I meant that:
> >>>>>>
> >>>>>> xen/arch/arm/blah.h would use ARM_BLAH_H
> >>>>>> xen/arch/x86/blah.h would use X86_BLAH_H
> >>>>>>
> >>>>>> You have a good question on something like xen/common/decompress.h and
> >>>>>> xen/common/event_channel.h.  What do you think about:
> >>>>>>
> >>>>>> COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H
> >>>>>>
> >>>>>> otherwise:
> >>>>>>
> >>>>>> XEN_BLAH_H, so specifically XEN_DECOMPRESS_H
> >>>>>>
> >>>>>> I prefer COMMON_BLAH_H but I think both versions are OK.
> >>>>>
> >>>>> IOW you disagree with my earlier "... and no path-derived prefixes",
> >>>>> and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
> >>>>> FTAOD my earlier suggestion was simply based on the observation that
> >>>>> the deeper the location of a header in the tree, the more unwieldy
> >>>>> its guard name would end up being if path prefixes were to be used.
> >>>>
> >>>> I don't have a strong opinion on "path-derived prefixes". I prefer
> >>>> consistency and easy-to-figure-out guidelines over shortness.
> > 
> > We adopted the MISRA Rule 5.4 which imposed us a limit (40 for Xen) on 
> > the number of characters for macros. AFAIU, this would apply to guards.
> > 
> > In the example provided by Jan (DRIVERS_PASSTHROUGH_VTD_DMAR_H), this is 
> > already 31 characters. So this is quite close to the limit.
> > 
> >>>>
> >>>> The advantage of a path-derived prefix is that it is trivial to figure
> >>>> out at first glance. If we can come up with another system that is also
> >>>> easy then fine. Do you have a suggestion? If so, sorry I missed it.
> >>>
> >>> Well, I kind of implicitly suggested "no path derived prefixes for private
> >>> headers", albeit realizing that there's a chance then of guards colliding.
> >>> I can't think of a good scheme which would fit all goals (no collisions,
> >>> uniformity, and not unduly long).
> >>
> >> Here I think we would benefit from a third opinion. Julien? Anyone?
> > 
> > Just to confirm, the opinion is only for private headers. You have an 
> > agreement for the rest, is it correct?

Yes


> > If so, then I think we need to have shorter names for guard to avoid 
> > hitting the 40 characters limit. I can't think of a way to have a common 
> > scheme between private and common headers. So I would consider to have a 
> > separate scheme.
> > 
> > I am not sure if you or Jan already proposed an alternative scheme.
> 
> Well, my suggestion was to derive from just the file name (no path
> components) for them. But I pointed out that this may lead to collisions
> when two or more private headers of the same name exist, and a CU ends
> up wanting to include any two of them. Adding in the leaf-most path
> component only might get us far enough to avoid collisions in practice,
> while at the same time not resulting in overly long guard names.

If I understood correctly I am fine with that. To make sure we are all
on the same page, can you provide a couple of samples?