Re: [PATCH] misra: add ASSERT_UNREACHABLE() in default clauses

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

On 8/12/25 00:21, Julien Grall wrote:
> Hi,
> 
> On 11/08/2025 21:30, Dmytro Prokopchuk1 wrote:
>> MISRA Rule 16.4: Every switch statement shall have a default label.
>> The default clause must contain either a statement or a comment
>> prior to its terminating break statement.
>>
>> However, there is a documented rule that apply to the Xen in
>> 'docs/misra/rules.rst':
>> Switch statements with integer types as controlling expression
>> should have a default label:
>>   - if the switch is expected to handle all possible cases
>>    explicitly, then a default label shall be added to handle
>>    unexpected error conditions, using BUG(), ASSERT(), WARN(),
>>    domain_crash(), or other appropriate methods;
>  > > These changes add `ASSERT_UNREACHABLE()` macro to the default 
> clause of
>> switch statements that already explicitly handle all possible cases. This
>> ensures compliance with MISRA, avoids undefined behavior in unreachable
>> paths, and helps detect errors during development.
>>
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
>> ---
>>   xen/arch/arm/decode.c      |  3 +++
>>   xen/arch/arm/guest_walk.c  |  4 ++++
>>   xen/common/grant_table.c   | 10 ++++++++--
>>   xen/drivers/char/console.c |  3 +++
>>   4 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
>> index 2537dbebc1..cb64137b3b 100644
>> --- a/xen/arch/arm/decode.c
>> +++ b/xen/arch/arm/decode.c
>> @@ -178,6 +178,9 @@ static int decode_thumb(register_t pc, struct 
>> hsr_dabt *dabt)
>>           case 3: /* Signed byte */
>>               update_dabt(dabt, reg, 0, true);
>>               break;
>> +        default:
>> +            ASSERT_UNREACHABLE();
>> +            break;
> 
> I am not sure about this one. Can't the compiler or even ECLAIR detect 
> that the value we match "opB & 3" and the 4 different values are handled?
> 
>  >           }>

The Eclair can handle such case with enum type.
I'm not sure that we want to apply such changes.

>>           break;
>> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
>> index 09fe486598..9199a29602 100644
>> --- a/xen/arch/arm/guest_walk.c
>> +++ b/xen/arch/arm/guest_walk.c
>> @@ -167,6 +167,10 @@ static bool guest_walk_sd(const struct vcpu *v,
>>               *perms |= GV2M_EXEC;
>>           break;
>> +
>> +        default:
>> +            ASSERT_UNREACHABLE();
>> +            break;
>>       }
>>       return true;
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index cf131c43a1..60fc47f0c8 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -330,9 +330,12 @@ shared_entry_header(struct grant_table *t, 
>> grant_ref_t ref)
>>           /* Returned values should be independent of speculative 
>> execution */
>>           block_speculation();
>>           return &shared_entry_v2(t, ref).hdr;
>> +
>> +    default:
>> +        ASSERT_UNREACHABLE();
>> +        break;
>>       }
>> -    ASSERT_UNREACHABLE();
>  >       block_speculation();>
>>       return NULL;
> 
> I know you are trying to apply the MISRA rule. But this is odd that you 
> move the ASSERT_UNREACHABLE() but then code after is still only 
> reachable from the default. In fact, this is introducing a risk if 
> someone decides to add a new case but then forgot to return a value.
> 
> By moving the two other lines, the compiler should be able to throw an 
> error if you forgot a return.
> 
> 
>> @@ -727,10 +730,13 @@ static unsigned int nr_grant_entries(struct 
>> grant_table *gt)
>>           /* Make sure we return a value independently of speculative 
>> execution */
>>           block_speculation();
>>           return f2e(nr_grant_frames(gt), 2);
>> +
>> +    default:
>> +        ASSERT_UNREACHABLE();
>> +        break;
>>   #undef f2e
>>       }
>> -    ASSERT_UNREACHABLE();
>>       block_speculation();
>>       return 0;
> 
> Same remark here.

Yes, 'default' case with appropriate comment will be better, I think.
Anyway, wrong 'gt_version' will be handled below by the 
ASSERT_UNREACHABLE().

> 
>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>> index 9bd5b4825d..608616f2af 100644
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -889,6 +889,9 @@ static int cf_check parse_console_timestamps(const 
>> char *s)
>>           opt_con_timestamp_mode = TSM_DATE;
>>           
>> con_timestamp_mode_upd(param_2_parfs(parse_console_timestamps));
>>           return 0;
>> +    default:
>> +        ASSERT_UNREACHABLE();
> 
> Looking at the implementation of parse_bool() it can return -1 if the 
> input provided is incorrect. So I don't think this path should contain 
> ASSERT_UNREACHABLE(). In fact, it should follow this directive:
> 
> "
>         - if the switch is expected to handle a subset of all possible
>           cases, then an empty default label shall be added with an
>           in-code comment on top of the default label with a reason and
>           when possible a more detailed explanation. Example::
> 
>               default:
>                   /* Notifier pattern */
>                   break;
> "

You are right, parse_bool() can return -1.
Approach: 'default' case with appropriate comment.

> 
> Cheers,
> 

Thanks!
Dmytro.