| Message ID | 3f1385f2ddb151a53ca092ea1caeac5d12fd80f5.1690985045.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address MISRA C:2012 Rule 2.1 | expand |
On 02.08.2023 16:38, Nicola Vetrini wrote: > --- 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; What use is "break" after "return"? IOW rather than adding code here, imo a line wants removing. Jan
On 03/08/2023 11:17, Jan Beulich wrote: > On 02.08.2023 16:38, Nicola Vetrini wrote: >> --- 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; > > What use is "break" after "return"? IOW rather than adding code here, > imo a line wants removing. > > Jan The "return false" after the switch would still be unreachable. The reasoning behind preserving the break is mainly MISRA Rule 16.3: "An unconditional break statement shall terminate every switch-clause", which has not yet been considered for adoption, but might be in future discussions, leading to putting back the break here.
On 07.08.2023 10:13, Nicola Vetrini wrote: > On 03/08/2023 11:17, Jan Beulich wrote: >> On 02.08.2023 16:38, Nicola Vetrini wrote: >>> --- 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; >> >> What use is "break" after "return"? IOW rather than adding code here, >> imo a line wants removing. > > The "return false" after the switch would still be unreachable. The > reasoning behind preserving the break > is mainly MISRA Rule 16.3: "An unconditional break statement shall > terminate every switch-clause", which has > not yet been considered for adoption, but might be in future > discussions, leading to putting back the break here. Well, adding such bogus "break"s is going to face my opposition, and it is in direct conflict with the "no unreachable code" rule here. Jan
Hi, 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). > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > The break in the clause is mandated by Required Rule 16.3, which is > not yet an accepted rule for Xen, but may be in the future. > --- > xen/common/sched/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c > index 022f548652..fcee902b4e 100644 > --- 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); Cheers,
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)? 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. 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). Jan
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,
>> >> ... "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, 16.3 is not Mandatory, just Required. I was just reluctant to knowingly add one more violation while fixing another before submitting the patch. If indeed 16.3 won't likely be adopted by Xen, then that break should indeed go away. However, the main thing that's holding me back from doing a slimmed-down v2 here is that evidently there's no consensus even on the ASSERT_UNREACHABLE() patches.
On 08/08/2023 16:53, Nicola Vetrini wrote: > >>> >>> ... "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, > > 16.3 is not Mandatory, just Required. Ah I misread what you wrote. In that case... > I was just reluctant to knowingly > add one more violation > while fixing another before submitting the patch. If indeed 16.3 won't > likely be adopted by Xen, > then that break should indeed go away. > > However, the main thing that's holding me back from doing a slimmed-down > v2 here is that > evidently there's no consensus even on the ASSERT_UNREACHABLE() patches. ... I agree with the following statement from Jan: "it should instead demand no [unannotated] fall-through, which can also be achieved by return, continue, and goto" Cheers,
On Tue, 8 Aug 2023, Julien Grall wrote: > On 08/08/2023 16:53, Nicola Vetrini wrote: > > > > ... "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, > > > > 16.3 is not Mandatory, just Required. > > Ah I misread what you wrote. In that case... > > > I was just reluctant to knowingly add one more violation > > while fixing another before submitting the patch. If indeed 16.3 won't > > likely be adopted by Xen, > > then that break should indeed go away. > > > > However, the main thing that's holding me back from doing a slimmed-down v2 > > here is that > > evidently there's no consensus even on the ASSERT_UNREACHABLE() patches. > > ... I agree with the following statement from Jan: > > "it should instead demand no [unannotated] fall-through, which can also be > achieved by return, continue, and goto" I also think it is a better idea to have no unannotated fall-through in switch statements (unless we have a "case" with nothing underneath, so two cases next to each other). In other words the following are all OK in my view: case A: do(); break; case B: do(); return true; case Ca: case Cb: goto fail; case D: i++; continue; case E: do(); /* fall-through */ case F: do(); break; While the following is not OK: case A: do(); case B: do(); This should be what we already do in Xen, although it is not documented properly. Jan, Julien do you agree? If so, could Eclair be configured to check for this pattern (or similar)? We must have one of the following: - break, continue, return, goto - /* fall-through */ - empty case body (case Ca/Cb)
On 08.08.2023 23:14, Stefano Stabellini wrote: > On Tue, 8 Aug 2023, Julien Grall wrote: >> On 08/08/2023 16:53, Nicola Vetrini wrote: >>>>> ... "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, >>> >>> 16.3 is not Mandatory, just Required. >> >> Ah I misread what you wrote. In that case... >> >>> I was just reluctant to knowingly add one more violation >>> while fixing another before submitting the patch. If indeed 16.3 won't >>> likely be adopted by Xen, >>> then that break should indeed go away. >>> >>> However, the main thing that's holding me back from doing a slimmed-down v2 >>> here is that >>> evidently there's no consensus even on the ASSERT_UNREACHABLE() patches. >> >> ... I agree with the following statement from Jan: >> >> "it should instead demand no [unannotated] fall-through, which can also be >> achieved by return, continue, and goto" > > I also think it is a better idea to have no unannotated fall-through in > switch statements (unless we have a "case" with nothing underneath, so > two cases next to each other). In other words the following are all OK > in my view: > > case A: > do(); > break; > case B: > do(); > return true; > case Ca: > case Cb: > goto fail; > case D: > i++; > continue; > case E: > do(); > /* fall-through */ > case F: > do(); > break; > > While the following is not OK: > > case A: > do(); > case B: > do(); > > This should be what we already do in Xen, although it is not documented > properly. Jan, Julien do you agree? Yes. > If so, could Eclair be configured to check for this pattern (or > similar)? > > We must have one of the following: > - break, continue, return, goto > - /* fall-through */ - fallthrough; (the pseudo keyword that we have; see compiler.h for actually having the documentation you're looking for, albeit missing "continue", and of course not really expected to live [just] there) Jan > - empty case body (case Ca/Cb)
Hi Stefano, On 08/08/2023 22:14, Stefano Stabellini wrote: > On Tue, 8 Aug 2023, Julien Grall wrote: >> On 08/08/2023 16:53, Nicola Vetrini wrote: >>>>> ... "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, >>> >>> 16.3 is not Mandatory, just Required. >> >> Ah I misread what you wrote. In that case... >> >>> I was just reluctant to knowingly add one more violation >>> while fixing another before submitting the patch. If indeed 16.3 won't >>> likely be adopted by Xen, >>> then that break should indeed go away. >>> >>> However, the main thing that's holding me back from doing a slimmed-down v2 >>> here is that >>> evidently there's no consensus even on the ASSERT_UNREACHABLE() patches. >> >> ... I agree with the following statement from Jan: >> >> "it should instead demand no [unannotated] fall-through, which can also be >> achieved by return, continue, and goto" > > I also think it is a better idea to have no unannotated fall-through in > switch statements (unless we have a "case" with nothing underneath, so > two cases next to each other). In other words the following are all OK > in my view: > > case A: > do(); > break; > case B: > do(); > return true; > case Ca: > case Cb: > goto fail; > case D: > i++; > continue; > case E: > do(); > /* fall-through */ > case F: > do(); > break; > > While the following is not OK: > > case A: > do(); > case B: > do(); > > This should be what we already do in Xen, although it is not documented > properly. Jan, Julien do you agree? I agree. Cheers,
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c index 022f548652..fcee902b4e 100644 --- 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);
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. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- The break in the clause is mandated by Required Rule 16.3, which is not yet an accepted rule for Xen, but may be in the future. --- xen/common/sched/core.c | 1 + 1 file changed, 1 insertion(+)