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

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

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.