Re: [PATCH] xen: address violation of MISRA C Rule 11.1

[Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>]

On 2024-12-13 11:08, Jan Beulich wrote:
> On 13.12.2024 01:53, Stefano Stabellini wrote:
> > On Thu, 12 Dec 2024, Jan Beulich wrote:
> > > On 12.12.2024 03:27, Stefano Stabellini wrote:
> > > > On Wed, 11 Dec 2024, Jan Beulich wrote:
> > > > > On 11.12.2024 12:02, Alessandro Zucchelli wrote:
> > > > > > Rule 11.1 states as following: "Conversions shall not be performed
> > > > > > between a pointer to a function and any other type".
> > > > > >
> > > > > > Functions "__machine_restart" and "__machine_halt" in 
> > > > > > "x86/shutdown.c"
> > > > > > and "halt_this_cpu" in "arm/shutdown.c" are defined as noreturn
> > > > > > functions and subsequently passed as parameters to function calls.
> > > > > > This violates the rule in Clang, where the "noreturn" attribute is
> > > > > > considered part of the function"s type.
> > > > > I'm unaware of build issues with Clang, hence can you clarify how 
> > > > > Clang's
> > > > > view comes into play here? In principle various attributes ought to 
> > > > > be
> > > > > part of a function's type; iirc that's also the case for gcc. Yet 
> > > > > how
> > > > > that matters to Eclair is still entirely unclear to me.
> > > > >
> > > > > > By removing the "noreturn"
> > > > > > attribbute and replacing it with uses of the ASSERT_UNREACHABLE 
> > > > > > macro,
> > > > > > these violations are addressed.
> > > > > Papered over, I'd say. What about release builds, for example?
> > > > >
> > > > > Deleting the attribute also has a clear downside 
> > > > > documentation-wise. If
> > > > > we really mean to remove them from what the compiler gets to see, I 
> > > > > think
> > > > > we ought to still retain them in commented-out shape.
> > > > Another option would be to #define noreturn to nothing for ECLAIR 
> > > > builds ?
> > > That again would feel like papering over things. Plus I don't know if 
> > > that's
> > > an option at all.
> > What is "papering over" and what is a "nice solution" is often up to 
> > the
> > personal opinions.
> >
> > From my point of view, Alessandro's patch doesn't make the code worse.
> > The ASSERT_UNREACHABLE solution is OK. I do agree with you that it
> > should not be required for us to remove "noreturn", but I don't think 
> > we
> > have used it consistently anyway across the Xen codebase.
> > ASSERT_UNREACHABLE is also a form of documentation that the function
> > does not return.
> >
> > In conclusion, I think all three options are acceptable:
> > 1) this patch as is
> > 2) this patch plus /* noreturn */ as a comment
> > 3) #define noreturn to nothing just for ECLAIR builds
> >
> > I don't mind either way, maybe option 2) is the best compromise.
> The variant with least impact on what we currently have (generated code
> wise) is 3), though, which hence would be my preference (well, not 
> exactly
> a preference, but the least bad one).
Another option could be to encapsulate these function pointer casts as 
follows:
#define REMOVE_NORETURN(x) (void(*)(void*))(x)
This approach allows us to retain the noreturn attribute and the 
associated optimizations;
note that the encapsulating macro will need to be deviated then.

--
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)