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_prokopch...@epam.com> >> --- >> 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.
