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(-)
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@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;
}
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;
@@ -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;
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") )
--
2.43.0On 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@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 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
On 11.08.2025 23:25, Nicola Vetrini wrote:
> On 2025-08-11 22:30, Dmytro Prokopchuk1 wrote:
>> --- 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
Did you also see Julien's reply? Imo, to address a complaint from one
rule, another rule is then being violated: The "default" label itself
is unreachable here.
Jan
On 2025-08-12 09:25, Jan Beulich wrote:
> On 11.08.2025 23:25, Nicola Vetrini wrote:
>> On 2025-08-11 22:30, Dmytro Prokopchuk1 wrote:
>>> --- 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
>
> Did you also see Julien's reply? Imo, to address a complaint from one
> rule, another rule is then being violated: The "default" label itself
> is unreachable here.
>
> Jan
Unfortunately only after sending my reply, however the point here is
that ASSERT_UNREACHABLE() is now considered as if it was not actually a
source of unreachability for any statement below (which is the case only
in configurations where NDEBUG is undefined iirc). This was done mainly
to allow stubs for Rule 2.1 so that their return statement just after an
ASSERT_UNREACHABLE() is not seen as a problem, but given that the
configuration to obtain that is global it influences treatment for other
rules as well, and its addition is relatively recent compared to the
text written in rules.rst.
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On 12.08.2025 11:55, Nicola Vetrini wrote:
> On 2025-08-12 09:25, Jan Beulich wrote:
>> On 11.08.2025 23:25, Nicola Vetrini wrote:
>>> On 2025-08-11 22:30, Dmytro Prokopchuk1 wrote:
>>>> --- 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
>>
>> Did you also see Julien's reply? Imo, to address a complaint from one
>> rule, another rule is then being violated: The "default" label itself
>> is unreachable here.
>
> Unfortunately only after sending my reply, however the point here is
> that ASSERT_UNREACHABLE() is now considered as if it was not actually a
> source of unreachability for any statement below (which is the case only
> in configurations where NDEBUG is undefined iirc). This was done mainly
> to allow stubs for Rule 2.1 so that their return statement just after an
> ASSERT_UNREACHABLE() is not seen as a problem, but given that the
> configuration to obtain that is global it influences treatment for other
> rules as well, and its addition is relatively recent compared to the
> text written in rules.rst.
I understand the special treatment of ASSERT_UNREACHABLE(). Yet even if that
wasn't there, both the default: label and the break; statement would be
unreachable here.
Jan
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@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?
> }>
> 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
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@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.
On 11.08.2025 23:21, Julien Grall wrote: > On 11/08/2025 21:30, Dmytro Prokopchuk1 wrote: >> --- 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. I think we did discuss this pattern in the past. While moving everything up to the "return" into the default: handling will please Eclair / Misra, we'll then end up with no return statement at the end of a non-void function. Beyond being good practice (imo) to have such a "main" return statement, that's actually another rule, just one we apparently didn't accept (15.5). Jan
Hi Jan, On 12/08/2025 08:32, Jan Beulich wrote: > On 11.08.2025 23:21, Julien Grall wrote: >> On 11/08/2025 21:30, Dmytro Prokopchuk1 wrote: >>> --- 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. > > I think we did discuss this pattern in the past. While moving everything up > to the "return" into the default: handling will please Eclair / Misra, we'll > then end up with no return statement at the end of a non-void function. > Beyond being good practice (imo) to have such a "main" return statement, > that's actually another rule, just one we apparently didn't accept (15.5). Reading 15.5, this seems to be about having a single return in the function. Unless I misunderstood something, this is different from what you suggest. Anyway, my main problem with this change is that ASSERT_UNREACHABLE() is moved. I could possibly settle with: default: break; } ASSERT_UNREACHABLE(); ... But at least to me, this pattern is more difficult to read because I have to look through the switch to understand the patch is only meant ot be used by the "default" case. Cheers, -- Julien Grall
On 13.08.2025 00:54, Julien Grall wrote: > Hi Jan, > > On 12/08/2025 08:32, Jan Beulich wrote: >> On 11.08.2025 23:21, Julien Grall wrote: >>> On 11/08/2025 21:30, Dmytro Prokopchuk1 wrote: >>>> --- 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. >> >> I think we did discuss this pattern in the past. While moving everything up >> to the "return" into the default: handling will please Eclair / Misra, we'll >> then end up with no return statement at the end of a non-void function. >> Beyond being good practice (imo) to have such a "main" return statement, >> that's actually another rule, just one we apparently didn't accept (15.5). > > Reading 15.5, this seems to be about having a single return in the > function. Unless I misunderstood something, this is different from what > you suggest. Sue, the connection is a lose one. What I mean is that adding yet another return _not_ at the end of the function moves use further away from 15.5 compliance. > Anyway, my main problem with this change is that ASSERT_UNREACHABLE() is > moved. I could possibly settle with: > > default: > break; > } Which would violate another rule, iirc (demanding that there be more than just "break"). > ASSERT_UNREACHABLE(); > ... > > But at least to me, this pattern is more difficult to read because I > have to look through the switch to understand the patch is only meant ot > be used by the "default" case. +1 Jan
On 8/13/25 01:54, Julien Grall wrote:
> Hi Jan,
>
> On 12/08/2025 08:32, Jan Beulich wrote:
>> On 11.08.2025 23:21, Julien Grall wrote:
>>> On 11/08/2025 21:30, Dmytro Prokopchuk1 wrote:
>>>> --- 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.
>>
>> I think we did discuss this pattern in the past. While moving
>> everything up
>> to the "return" into the default: handling will please Eclair / Misra,
>> we'll
>> then end up with no return statement at the end of a non-void function.
>> Beyond being good practice (imo) to have such a "main" return statement,
>> that's actually another rule, just one we apparently didn't accept
>> (15.5).
>
> Reading 15.5, this seems to be about having a single return in the
> function. Unless I misunderstood something, this is different from what
> you suggest.
>
> Anyway, my main problem with this change is that ASSERT_UNREACHABLE() is
> moved. I could possibly settle with:
>
> default:
> break;
> }
>
> ASSERT_UNREACHABLE();
> ...
>
> But at least to me, this pattern is more difficult to read because I
> have to look through the switch to understand the patch is only meant ot
> be used by the "default" case.
>
> Cheers,
>
Hi all!
Let's summarize the discussion.
1. There are two cases, where `switch' statement has a controlling
value that is completely covered by its labeled statements.
Here:
switch ( opB & 0x3 )
And here:
switch ( pte.walk.dt )
Originally it violates rule 16.4 "`switch' statement has no `default'
labels".
Well, adding empty
default: break;
is not a solution, because it starts to violate rule 2.1 "`break'
statement is unreachable".
Adding
default: ASSERT_UNREACHABLE(); break;
looks fine from Eclair point of view, but actually `default' case is
still unreachable.
I think the easiest way is to insert SAF-xx marker instead of the
`default' case.
Changing these to enums - not fine as for me.
Maybe there is a way to configure Eclair to ignore the rule 16.4 in such
cases.
Maybe Nicola can suggest something...
2. Regarding moving ASSERT_UNREACHABLE(); inside `default' case.
Actually switch check Granttable version.
switch ( gt->gt_version )
And it must be '1' or '2'. Other values are wrong.
I think `default' case should be with assert:
default:
ASSERT(false);
return;
This can catch wrong 'gt_version' values.
Dmytro.
© 2016 - 2025 Red Hat, Inc.