Re: [XEN PATCH 5/7] xen/arm: v{cp,sys}reg: address violations of MISRA C:2012 Rule 16.3

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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