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
