[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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |