Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Julien Grall <julien@xxxxxxx>]

Hi,

On 08/08/2023 16:36, Jan Beulich wrote:
> On 08.08.2023 17:25, Julien Grall wrote:
> > On 02/08/2023 15:38, Nicola Vetrini wrote:
> > > The break statement after the return statement is definitely unreachable.
> > > As such, an call to the ASSERT_UNREACHABLE() macro is added to signal
> > > the intentionality of such construct.
> > How about using unreachable() rather than ASSERT_UNREACHABLE()? The main
> > difference is the later will hint the compiler that the code cannot be
> > reached and will not crash Xen in debug build (this could be changed).
> Isn't using unreachable() in place of ASSERT_UNREACHABLE() unsafe (not
> here but in general)?
Yes it is.

 It'll tell the compiler that (in the extreme case)
> it can omit the function epilogue, including the return instruction. In
> the resulting build, if the code turns out to be reachable, execution
> would fall through into whatever follows.
Right, but the problem is somewhat similar with adding 
ASSERT_UNREACHABLE() without proper error path because there is no 
guarantee the rest of the code will be correct in the unlikely case it 
is reachable.
> In the case here ...
>
> > > --- a/xen/common/sched/core.c
> > > +++ b/xen/common/sched/core.c
> > > @@ -2233,6 +2233,7 @@ static bool sched_tasklet_check_cpu(unsigned int cpu)
> > >            /* fallthrough */
> > >        case TASKLET_enqueued|TASKLET_scheduled:
> > >            return true;
> > > +        ASSERT_UNREACHABLE();
> > >            break;
> > >        case TASKLET_scheduled:
> > >            clear_bit(_TASKLET_scheduled, tasklet_work);
> ... "return" alone already tells the compiler that "break" is
> unreachable. You don't need unreachable() for that. As said before,
> "break" like this simply want purging (and Misra is wrong to demand
> "break" everywhere - it should instead demand no [unannotated]
> fall-through, which can also be achieved by return, continue, and
> goto).
My suggestion was in the context of this series where we add 
ASSERT_UNREACHABLE() before break. From my understanding, we don't have 
a lot of leeway here because, from what Nicola wrote, rule 16.3 is 
mandatory.
So I think using unreachable is better if we every decide to use break 
after return.
Cheers,

--
Julien Grall