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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 16 Dec 2024, Jan Beulich wrote:
> On 13.12.2024 15:02, 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.
> 
> And then have one such macro for every attribute that may need zapping?
> What if there are multiple? Any macro may do, yet which one to use would
> be unclear. What if only some attributes need zapping, and some need
> retaining?

It is always a judgment call between addressing issues ad hoc and
developing generic solutions. In this case, since we are discussing only
one attribute, I do not think we should attempt to generalize it
further.