Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2023-12-11 17:05, Julien Grall wrote:
> On 11/12/2023 15:59, Julien Grall wrote:
>> Hi Nicola,
>> 
>> On 11/12/2023 14:54, Nicola Vetrini wrote:
>>> On 2023-12-11 13:32, Julien Grall wrote:
>>>> Hi,
>>>> 
>>>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>>>> The branches of the switch after a call to 'do_unexpected_trap'
>>>>> cannot return, but there is one path that may return, hence
>>>>> only some clauses are marked with ASSERT_UNREACHABLE().
>>>> I don't understand why this is necessary. The code should never be 
>>>> reachable because do_unexpected_trap() is a noreturn().
>>>> 
>>>> Cheers,
>>> 
>>> It was meant as a safeguard against mistakes.
>> 
>> I am confused about which mistakes you are trying to prevent. Are you 
>> saying you are not trusting the noreturn attribute?
>> 
>> And if so, are you intending to add ASSERT_UNREACHABLE() after every 
>> single call to noreturn functions?
> 
> Replying to myself. What's confusing the most is that in [1], you 
> decided to not add the ASSERT_UNREACHABLE(). Yet the problem is 
> similar.
> 
> I'd also like to point out that by removing the "break", then if the 
> 'noreturn' function turns out to return, then in prod build you would 
> fallthrough to the next case. And who knows what's going to happen...
> 
> All of this really adds some confusion...
> 

I should have checked before responding: do_trap_hyp_sync is not afaik 
noreturn. Specifically, do_trap_brk may return. If I worked under the 
wrong assumption, then certainly the ASSERT_UNREACHABLE-s should be 
dropped.

>> 
>>> There are MISRA rules that deal with this aspect (e.g., a noreturn 
>>> function should not return), but they are not in Amendment 2, which 
>>> is what Xen is following.
>> Do you mind providing a bit more details about the rules you are 
>> talking about? At least the numbers would be helpful.
> 
> [1] 
> https://lore.kernel.org/xen-devel/c0a8a12e39d688e101936d221af0f8eeefabe352.1702283415.git.nicola.vetrini@bugseng.com/
> 
> Cheers,

Sure. Here are the rules concerned with _Noreturn:

R17.9: A function declared with a _Noreturn function specifier shall not 
return to its caller
R17.10: A function declared with a _Noreturn function specifier shall 
have void return type
R17.11: A function that never returns should be declared with a 
_Noreturn function specifier

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)