| Message ID | 541bc4fd47d26b12ea131590bf0c49f7c92d9368.1703066935.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: address violations of MISRA C:2012 Rule 16.3 | expand |
Hi Federico, On 20/12/2023 11:03, Federico Serafini wrote: > Refactor of the switch-clauses to have a return statement at the end. > This satisfies the requirements to deviate Rule 16.3 ("An unconditional > `break' statement shall terminate every switch-clause). > No functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > --- > xen/arch/arm/arm64/vsysreg.c | 4 ++-- > xen/arch/arm/vcpreg.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c > index b5d54c569b..247f08ad8d 100644 > --- a/xen/arch/arm/arm64/vsysreg.c > +++ b/xen/arch/arm/arm64/vsysreg.c > @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, > /* RO at EL0. RAZ/WI at EL1 */ > if ( regs_mode_is_user(regs) ) > return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); > - else > - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); > + > + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); I don't 100% like this change (mostly because I find if/else clearer here). But I have the feeling any other solution would probably be worse. So: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
On 20.12.2023 12:48, Julien Grall wrote: > On 20/12/2023 11:03, Federico Serafini wrote: >> --- a/xen/arch/arm/arm64/vsysreg.c >> +++ b/xen/arch/arm/arm64/vsysreg.c >> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, >> /* RO at EL0. RAZ/WI at EL1 */ >> if ( regs_mode_is_user(regs) ) >> return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); >> - else >> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >> + >> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); > > I don't 100% like this change (mostly because I find if/else clearer > here). While (it doesn't matter here) my view on this is different, I'm still puzzled why the tool would complain / why a change here is necessary. It is not _one_ return statement, but there's still (and obviously) no way of falling through. > But I have the feeling any other solution would probably be > worse. Use the conditional operator? Jan > So: > > Acked-by: Julien Grall <jgrall@amazon.com> > > Cheers, >
On 20/12/23 12:55, Jan Beulich wrote: > On 20.12.2023 12:48, Julien Grall wrote: >> On 20/12/2023 11:03, Federico Serafini wrote: >>> --- a/xen/arch/arm/arm64/vsysreg.c >>> +++ b/xen/arch/arm/arm64/vsysreg.c >>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, >>> /* RO at EL0. RAZ/WI at EL1 */ >>> if ( regs_mode_is_user(regs) ) >>> return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); >>> - else >>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >>> + >>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >> >> I don't 100% like this change (mostly because I find if/else clearer >> here). > > While (it doesn't matter here) my view on this is different, I'm still > puzzled why the tool would complain / why a change here is necessary. > It is not _one_ return statement, but there's still (and obviously) no > way of falling through. The tool is configurable: if you prefer deviate these cases instead of refactoring the code I can update the configuration. >> But I have the feeling any other solution would probably be >> worse. > > Use the conditional operator? > > Jan > >> So: >> >> Acked-by: Julien Grall <jgrall@amazon.com> >> >> Cheers, >> >
On Wed, 20 Dec 2023, Federico Serafini wrote: > On 20/12/23 12:55, Jan Beulich wrote: > > On 20.12.2023 12:48, Julien Grall wrote: > > > On 20/12/2023 11:03, Federico Serafini wrote: > > > > --- a/xen/arch/arm/arm64/vsysreg.c > > > > +++ b/xen/arch/arm/arm64/vsysreg.c > > > > @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, > > > > /* RO at EL0. RAZ/WI at EL1 */ > > > > if ( regs_mode_is_user(regs) ) > > > > return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, > > > > 0); > > > > - else > > > > - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, > > > > 1); > > > > + > > > > + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); > > > > > > I don't 100% like this change (mostly because I find if/else clearer > > > here). > > > > While (it doesn't matter here) my view on this is different, I'm still > > puzzled why the tool would complain / why a change here is necessary. > > It is not _one_ return statement, but there's still (and obviously) no > > way of falling through. > > The tool is configurable: > if you prefer deviate these cases instead of refactoring the code > I can update the configuration. If you say "deviation", it implies that there is a violation. I think Jan's point was that both code versions shouldn't be any different. # option-1 if (a) return f1(); else return f2(); # option-2 if (a) return f1(); return f2(); Both options are equally guaranteed to always return. It looks like a peculiar limitation to only recognize option-2 as something that returns 100% of the times. Also option-1 returns 100% of the times. What am I missing?
On 20/12/2023 6:24 pm, Stefano Stabellini wrote: > On Wed, 20 Dec 2023, Federico Serafini wrote: >> On 20/12/23 12:55, Jan Beulich wrote: >>> On 20.12.2023 12:48, Julien Grall wrote: >>>> On 20/12/2023 11:03, Federico Serafini wrote: >>>>> --- a/xen/arch/arm/arm64/vsysreg.c >>>>> +++ b/xen/arch/arm/arm64/vsysreg.c >>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, >>>>> /* RO at EL0. RAZ/WI at EL1 */ >>>>> if ( regs_mode_is_user(regs) ) >>>>> return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, >>>>> 0); >>>>> - else >>>>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, >>>>> 1); >>>>> + >>>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >>>> I don't 100% like this change (mostly because I find if/else clearer >>>> here). >>> While (it doesn't matter here) my view on this is different, I'm still >>> puzzled why the tool would complain / why a change here is necessary. >>> It is not _one_ return statement, but there's still (and obviously) no >>> way of falling through. >> The tool is configurable: >> if you prefer deviate these cases instead of refactoring the code >> I can update the configuration. > > If you say "deviation", it implies that there is a violation. I think > Jan's point was that both code versions shouldn't be any different. > > # option-1 > if (a) > return f1(); > else > return f2(); > > # option-2 > if (a) > return f1(); > return f2(); > > Both options are equally guaranteed to always return. It looks like a > peculiar limitation to only recognize option-2 as something that returns > 100% of the times. Also option-1 returns 100% of the times. What am I > missing? For completeness, it's worth saying that there is an option-3: return a ? f1() : f2(); which is very clearly only a single return, but I personally don't like this as I often find it to be less clear than either other option. All options have a guaranteed return, but there cases including this one where option-1 is clearest way to write the logic. ~Andrew
On 20.12.2023 13:15, Federico Serafini wrote: > On 20/12/23 12:55, Jan Beulich wrote: >> On 20.12.2023 12:48, Julien Grall wrote: >>> On 20/12/2023 11:03, Federico Serafini wrote: >>>> --- a/xen/arch/arm/arm64/vsysreg.c >>>> +++ b/xen/arch/arm/arm64/vsysreg.c >>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, >>>> /* RO at EL0. RAZ/WI at EL1 */ >>>> if ( regs_mode_is_user(regs) ) >>>> return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); >>>> - else >>>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >>>> + >>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >>> >>> I don't 100% like this change (mostly because I find if/else clearer >>> here). >> >> While (it doesn't matter here) my view on this is different, I'm still >> puzzled why the tool would complain / why a change here is necessary. >> It is not _one_ return statement, but there's still (and obviously) no >> way of falling through. > > The tool is configurable: > if you prefer deviate these cases instead of refactoring the code > I can update the configuration. I guess this then needs to be discussed on the first call in the new year. Stefano - can you take note of that, please? Jan
On 20/12/23 22:23, Andrew Cooper wrote: > On 20/12/2023 6:24 pm, Stefano Stabellini wrote: >> On Wed, 20 Dec 2023, Federico Serafini wrote: >>> On 20/12/23 12:55, Jan Beulich wrote: >>>> On 20.12.2023 12:48, Julien Grall wrote: >>>>> On 20/12/2023 11:03, Federico Serafini wrote: >>>>>> --- a/xen/arch/arm/arm64/vsysreg.c >>>>>> +++ b/xen/arch/arm/arm64/vsysreg.c >>>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, >>>>>> /* RO at EL0. RAZ/WI at EL1 */ >>>>>> if ( regs_mode_is_user(regs) ) >>>>>> return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, >>>>>> 0); >>>>>> - else >>>>>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, >>>>>> 1); >>>>>> + >>>>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); >>>>> I don't 100% like this change (mostly because I find if/else clearer >>>>> here). >>>> While (it doesn't matter here) my view on this is different, I'm still >>>> puzzled why the tool would complain / why a change here is necessary. >>>> It is not _one_ return statement, but there's still (and obviously) no >>>> way of falling through. >>> The tool is configurable: >>> if you prefer deviate these cases instead of refactoring the code >>> I can update the configuration. >> >> If you say "deviation", it implies that there is a violation. I think >> Jan's point was that both code versions shouldn't be any different. >> >> # option-1 >> if (a) >> return f1(); >> else >> return f2(); >> >> # option-2 >> if (a) >> return f1(); >> return f2(); >> >> Both options are equally guaranteed to always return. It looks like a >> peculiar limitation to only recognize option-2 as something that returns >> 100% of the times. Also option-1 returns 100% of the times. What am I >> missing? I don't think this is necessarily a limitation because it highlights cases where an if-else could be replaced with a simple if: some may find an if-else clearer, other may find the single if clearer. From a safety point of view both options are safe because there is no risk of unintentional fall through. If you all agree on the fact that small code refactoring like the ones I proposed are counterproductive, then I can update the tool configuration to consider also option-1 as safe. > > For completeness, it's worth saying that there is an option-3: > > return a ? f1() : f2(); > > which is very clearly only a single return, but I personally don't like > this as I often find it to be less clear than either other option. Option-3 is currently considered as safe. > > All options have a guaranteed return, but there cases including this one > where option-1 is clearest way to write the logic. > > ~Andrew
On 21/12/2023 10:29 am, Federico Serafini wrote: > On 20/12/23 22:23, Andrew Cooper wrote: >> On 20/12/2023 6:24 pm, Stefano Stabellini wrote: >>> On Wed, 20 Dec 2023, Federico Serafini wrote: >>>> On 20/12/23 12:55, Jan Beulich wrote: >>>>> On 20.12.2023 12:48, Julien Grall wrote: >>>>>> On 20/12/2023 11:03, Federico Serafini wrote: >>>>>>> --- a/xen/arch/arm/arm64/vsysreg.c >>>>>>> +++ b/xen/arch/arm/arm64/vsysreg.c >>>>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, >>>>>>> /* RO at EL0. RAZ/WI at EL1 */ >>>>>>> if ( regs_mode_is_user(regs) ) >>>>>>> return handle_ro_raz(regs, regidx, >>>>>>> hsr.sysreg.read, hsr, >>>>>>> 0); >>>>>>> - else >>>>>>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, >>>>>>> hsr, >>>>>>> 1); >>>>>>> + >>>>>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, >>>>>>> hsr, 1); >>>>>> I don't 100% like this change (mostly because I find if/else clearer >>>>>> here). >>>>> While (it doesn't matter here) my view on this is different, I'm >>>>> still >>>>> puzzled why the tool would complain / why a change here is necessary. >>>>> It is not _one_ return statement, but there's still (and >>>>> obviously) no >>>>> way of falling through. >>>> The tool is configurable: >>>> if you prefer deviate these cases instead of refactoring the code >>>> I can update the configuration. >>> If you say "deviation", it implies that there is a violation. I think >>> Jan's point was that both code versions shouldn't be any different. >>> >>> # option-1 >>> if (a) >>> return f1(); >>> else >>> return f2(); >>> >>> # option-2 >>> if (a) >>> return f1(); >>> return f2(); >>> >>> Both options are equally guaranteed to always return. It looks like a >>> peculiar limitation to only recognize option-2 as something that >>> returns >>> 100% of the times. Also option-1 returns 100% of the times. What am I >>> missing? > > I don't think this is necessarily a limitation because it highlights > cases where an if-else could be replaced with a simple if: > some may find an if-else clearer, other may find the single if clearer. > > From a safety point of view both options are safe because there > is no risk of unintentional fall through. > > If you all agree on the fact that small code refactoring like the ones I > proposed are counterproductive, then I can update the tool configuration > to consider also option-1 as safe. I would certainly prefer if we could continue to use option 1. ~Andrew
On Thu, 21 Dec 2023, Andrew Cooper wrote: > On 21/12/2023 10:29 am, Federico Serafini wrote: > > On 20/12/23 22:23, Andrew Cooper wrote: > >> On 20/12/2023 6:24 pm, Stefano Stabellini wrote: > >>> On Wed, 20 Dec 2023, Federico Serafini wrote: > >>>> On 20/12/23 12:55, Jan Beulich wrote: > >>>>> On 20.12.2023 12:48, Julien Grall wrote: > >>>>>> On 20/12/2023 11:03, Federico Serafini wrote: > >>>>>>> --- a/xen/arch/arm/arm64/vsysreg.c > >>>>>>> +++ b/xen/arch/arm/arm64/vsysreg.c > >>>>>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, > >>>>>>> /* RO at EL0. RAZ/WI at EL1 */ > >>>>>>> if ( regs_mode_is_user(regs) ) > >>>>>>> return handle_ro_raz(regs, regidx, > >>>>>>> hsr.sysreg.read, hsr, > >>>>>>> 0); > >>>>>>> - else > >>>>>>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, > >>>>>>> hsr, > >>>>>>> 1); > >>>>>>> + > >>>>>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, > >>>>>>> hsr, 1); > >>>>>> I don't 100% like this change (mostly because I find if/else clearer > >>>>>> here). > >>>>> While (it doesn't matter here) my view on this is different, I'm > >>>>> still > >>>>> puzzled why the tool would complain / why a change here is necessary. > >>>>> It is not _one_ return statement, but there's still (and > >>>>> obviously) no > >>>>> way of falling through. > >>>> The tool is configurable: > >>>> if you prefer deviate these cases instead of refactoring the code > >>>> I can update the configuration. > >>> If you say "deviation", it implies that there is a violation. I think > >>> Jan's point was that both code versions shouldn't be any different. > >>> > >>> # option-1 > >>> if (a) > >>> return f1(); > >>> else > >>> return f2(); > >>> > >>> # option-2 > >>> if (a) > >>> return f1(); > >>> return f2(); > >>> > >>> Both options are equally guaranteed to always return. It looks like a > >>> peculiar limitation to only recognize option-2 as something that > >>> returns > >>> 100% of the times. Also option-1 returns 100% of the times. What am I > >>> missing? > > > > I don't think this is necessarily a limitation because it highlights > > cases where an if-else could be replaced with a simple if: > > some may find an if-else clearer, other may find the single if clearer. > > > > From a safety point of view both options are safe because there > > is no risk of unintentional fall through. > > > > If you all agree on the fact that small code refactoring like the ones I > > proposed are counterproductive, then I can update the tool configuration > > to consider also option-1 as safe. > > I would certainly prefer if we could continue to use option 1. Yes, that is my preference too
On Thu, 21 Dec 2023, Jan Beulich wrote: > On 20.12.2023 13:15, Federico Serafini wrote: > > On 20/12/23 12:55, Jan Beulich wrote: > >> On 20.12.2023 12:48, Julien Grall wrote: > >>> On 20/12/2023 11:03, Federico Serafini wrote: > >>>> --- a/xen/arch/arm/arm64/vsysreg.c > >>>> +++ b/xen/arch/arm/arm64/vsysreg.c > >>>> @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, > >>>> /* RO at EL0. RAZ/WI at EL1 */ > >>>> if ( regs_mode_is_user(regs) ) > >>>> return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); > >>>> - else > >>>> - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); > >>>> + > >>>> + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); > >>> > >>> I don't 100% like this change (mostly because I find if/else clearer > >>> here). > >> > >> While (it doesn't matter here) my view on this is different, I'm still > >> puzzled why the tool would complain / why a change here is necessary. > >> It is not _one_ return statement, but there's still (and obviously) no > >> way of falling through. > > > > The tool is configurable: > > if you prefer deviate these cases instead of refactoring the code > > I can update the configuration. > > I guess this then needs to be discussed on the first call in the new year. > Stefano - can you take note of that, please? Yes, will do
diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c index b5d54c569b..247f08ad8d 100644 --- a/xen/arch/arm/arm64/vsysreg.c +++ b/xen/arch/arm/arm64/vsysreg.c @@ -210,8 +210,8 @@ void do_sysreg(struct cpu_user_regs *regs, /* RO at EL0. RAZ/WI at EL1 */ if ( regs_mode_is_user(regs) ) return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0); - else - return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); + + return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1); case HSR_SYSREG_PMCR_EL0: case HSR_SYSREG_PMCNTENSET_EL0: case HSR_SYSREG_PMCNTENCLR_EL0: diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c index a2d0500704..685609f825 100644 --- a/xen/arch/arm/vcpreg.c +++ b/xen/arch/arm/vcpreg.c @@ -289,8 +289,8 @@ void do_cp15_32(struct cpu_user_regs *regs, const union hsr hsr) /* RO at EL0. RAZ/WI at EL1 */ if ( regs_mode_is_user(regs) ) return handle_ro_raz(regs, regidx, cp32.read, hsr, 0); - else - return handle_raz_wi(regs, regidx, cp32.read, hsr, 1); + + return handle_raz_wi(regs, regidx, cp32.read, hsr, 1); case HSR_CPREG32(PMINTENSET): case HSR_CPREG32(PMINTENCLR): /* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */
Refactor of the switch-clauses to have a return statement at the end. This satisfies the requirements to deviate Rule 16.3 ("An unconditional `break' statement shall terminate every switch-clause). No functional change. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/arch/arm/arm64/vsysreg.c | 4 ++-- xen/arch/arm/vcpreg.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)