Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-12-11 10:30, Jan Beulich wrote:
> On 11.12.2025 10:15, Nicola Vetrini wrote:
> > On 2025-12-11 09:36, Jan Beulich wrote:
> > > On 10.12.2025 19:30, Andrew Cooper wrote:
> > > > With the wider testing, some more violations have been spotted.  
> > > > This
> > > > addresses violations of Rule 20.7 which requires macro parameters to
> > > > be
> > > > bracketed.
> > > >
> > > > No functional change.
> > > >
> > > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > ---
> > > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > CC: consulting@xxxxxxxxxxx <consulting@xxxxxxxxxxx>
> > > > CC: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > > > ---
> > > >  xen/arch/x86/mm/shadow/multi.c     | 2 +-
> > > >  xen/arch/x86/mm/shadow/private.h   | 6 +++---
> > > >  xen/drivers/passthrough/vtd/dmar.h | 2 +-
> > > >  xen/include/xen/kexec.h            | 4 ++--
> > > >  4 files changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/xen/arch/x86/mm/shadow/multi.c
> > > > b/xen/arch/x86/mm/shadow/multi.c
> > > > index 03be61e225c0..36ee6554b4c4 100644
> > > > --- a/xen/arch/x86/mm/shadow/multi.c
> > > > +++ b/xen/arch/x86/mm/shadow/multi.c
> > > > @@ -781,7 +781,7 @@ do {
> > > >                      \
> > > >          (_sl1e) = _sp + _i;
> > > >   \
> > > >          if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT )
> > > >   \
> > > >              {_code}
> > > >   \
> > > > -        if ( _done ) break;
> > > >   \
> > > > +        if ( (_done) ) break;
> > > >   \
> > > I don't understand this: There are parentheses already from if()
> > > itself.
> > Yeah, syntactically there are, but those are parsed as part of the if,
> > rather than its condition; the AST node contained within does not have
> > parentheses around it.
> I fear I don't follow. Besides us not using parentheses elsewhere when
> if() is used like this macros, the point of requiring parentheses is 
> (aiui)
> to make precedence explicit. There already is no ambiguity here due to 
> the
> syntactically require parentheses in if(). Why would a rule and/or the
> tool require redundant ones?
this is parsed as (more or less) "if_stmt(integer_literal(0))" rather 
than "if_stmt(paren_expr(integer_literal(0)))" when the macro is invoked 
with 0 as parameter _done. Now, syntactically the parentheses are in the 
source code, so the letter of the rule is satisfied (as long as there is 
a single condition in the if condition), but the presence of those 
parentheses is lost when parsing. I see how this can be seen as a false 
positive, and we will definitely add some special handling so that cases 
like this are properly recognized, but for simplicity here I would add 
some extra parentheses, at least until the false positive is not 
resolved
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253