Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-03-07 08:42, Jan Beulich wrote:
> On 07.03.2024 02:39, Stefano Stabellini wrote:
> > On Tue, 5 Mar 2024, Jan Beulich wrote:
> > > On 05.03.2024 03:03, Stefano Stabellini wrote:
> > > > On Mon, 4 Mar 2024, Jan Beulich wrote:
> > > > > On 02.03.2024 02:37, Stefano Stabellini wrote:
> > > > > > On Fri, 1 Mar 2024, Jan Beulich wrote:
> > > > > > > On 29.02.2024 23:57, Stefano Stabellini wrote:
> > > > > > > > On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> > > > > > > > > MISRA C Rule 20.7 states: "Expressions resulting from the 
> > > > > > > > > expansion
> > > > > > > > > of macro parameters shall be enclosed in parentheses". 
> > > > > > > > > Therefore, some
> > > > > > > > > macro definitions should gain additional parentheses to ensure 
> > > > > > > > > that all
> > > > > > > > > current and future users will be safe with respect to 
> > > > > > > > > expansions that
> > > > > > > > > can possibly alter the semantics of the passed-in macro 
> > > > > > > > > parameter.
> > > > > > > > > No functional change.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > > > > > > > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > > > You did see the discussion on earlier patches, though? I don't 
> > > > > > > think
> > > > > > > any of the parentheses here are needed or wanted.
> > > > > > We need to align on this. Currently if we go by what's written in
> > > > > > docs/misra/deviations.rst, then rhs should have parentheses.
> > > > > Quoting the actual patch again:
> > > > [...]
> > > >
> > > > > What rhs are you talking about in light of this change? The only 
> > > > > rhs I
> > > > > can spot here is already parenthesized.
> > > > Yes you are right. I replied here as an overall comment about our
> > > > approach to 20.7, although this patch is not a good example. My 
> > > > reply
> > > > was meant in the context of 
> > > > https://marc.info/?l=xen-devel&m=170928051025701
> > > I'm still confused: The rhs is being parenthsized there. It's the 
> > > _lhs_
> > > which isn't and ...
> > >
> > > > > > Can we safely claim that rhs parentheses are never needed? If so, 
> > > > > > then
> > > > > > great, let's add it to deviations.rst and skip them here and other
> > > > > > places in this patch series (e.g. patch #8). When I say "never" I 
> > > > > > am
> > > > > > taking for granted that the caller is not doing something 
> > > > > > completely
> > > > > > unacceptably broken such as:
> > > > > >
> > > > > >      WRITE_SYSREG64(var +, TTBR0_EL1)
> > > > > I'm afraid I can't associate this with the patch here either. 
> > > > > Instead in
> > > > > the context here a (respective) construct as you mention above 
> > > > > would simply
> > > > > fail to build.
> > > > Fair enough it will break the build. I was trying to clarify that 
> > > > when I
> > > > wrote "the rhs parentheses are never needed" I meant "never" within
> > > > reason. One can always find ways to break the system and I tried to 
> > > > make
> > > > an example of something that for sure would break rhs or lhs without
> > > > parentheses.
> > > >
> > > > I meant to say, if we don't account for exceptionally broken cases, 
> > > > can
> > > > we safety say we don't need parentheses for rhs?
> > > ... doesn't need to, unless - as you say - one contrives examples. 
> > > Yet to
> > > clarify here as well: I assume you mean "we don't need parentheses 
> > > for lhs".
> > Yes. Far clarity, we are all aligned that lhs does not need 
> > parentheses
> > and in fact it is even already deviated in docs/misra/deviations.rst.
> >
> > Now back to this specific patch.
> >
> > The problem is that the comma ',' doesn't need parenthesis for the
> > parameters. However, the way we wrote the deviation in
> > docs/misra/deviations.rst doesn't cover the case this patch is dealing
> > with. docs/misra/deviations.rst only speaks of functions and macros
> > arguments.
> >
> > Should we rephrase docs/misra/deviations.rst in terms of ',' instead ?
> That's what I've requested elsewhere, yes.
>
> > Can ECLAR deal with it?
> I seem to vaguely recall Nicola saying it can, but if not then it 
> surely
> can be made to do so.
>
> Jan
Yes. In v2 I'll write the deviation, so that this patch and other can be 
dropped, since they only deal with this subcase.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)