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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-08-11 22: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 think this is fine, and there should be no problems with the break 
being unreachable in some configs due to the call property for 
ASSERT_UNREACHABLE
-doc_begin="Calls to function `__builtin_unreachable()' in the expansion 
of macro
`ASSERT_UNREACHABLE()' are not considered to have the `noreturn' 
property."
-call_properties+={"name(__builtin_unreachable)&&stmt(begin(any_exp(macro(name(ASSERT_UNREACHABLE)))))", 
{"noreturn(false)"}}
-doc_end


>          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;
>      }
This one instead, besides being indented misleadingly IMO, should 
instead be on an enum instead of
/*
 * First level translation table descriptor types used by the AArch32
 * short-descriptor translation table format.
 */
#define L1DESC_INVALID                      (0)
#define L1DESC_PAGE_TABLE                   (1)
#define L1DESC_SECTION                      (2)
#define L1DESC_SECTION_PXN                  (3)

so that

-doc_begin="Switch statements having a controlling expression of enum 
type deliberately do not have a default case: gcc -Wall enables -Wswitch 
which warns (and breaks the build as we use -Werror) if one of the enum 
labels is missing from the switch."
-config=MC3A2.R16.4,reports+={deliberate,'any_area(kind(context)&&^.* 
has no 
`default.*$&&stmt(node(switch_stmt)&&child(cond,skip(__non_syntactic_paren_stmts,type(canonical(enum_underlying_type(any())))))))'}
-doc_end

applies. What do you think?

>      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();
This is ok I think, same as (1).

>      return NULL;
> @@ -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();
Same here.

>      return 0;
> 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();
> +        break;
>      }
>      if ( *s == '\0' || /* Compat for old booleanparam() */
>           !strcmp(s, "date") )
And here as well.

--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253