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.
