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

[Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>]

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)