[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.