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

[Julien Grall <julien@xxxxxxx>]

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?
>           }>
>           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.

> 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;
"

Cheers,

--
Julien Grall