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

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

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
--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)