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
