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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 13 Dec 2024, Alessandro Zucchelli wrote:
> 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.

I think that's OK.