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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-12-11 11:38, Nicola Vetrini wrote:
> 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
Actually giving this a closer look the tool is correct: the fully 
expanded code is:
 19562     }} if ( ({ (__done = done); }) ) break; 
increment_ptr_to_guest_entry(((void*)0)); } unmap_domain_page(_sp); } 
while
                                <~~>

so the "done" argument ends up being expanded without parentheses, hence 
the report is correct and the extra parentheses are needed, but should 
be put into
/* 32-bit l1, on PAE or 64-bit shadows: need to walk both pages of 
shadow */
   791 #if GUEST_PAGING_LEVELS == 2 && SHADOW_PAGING_LEVELS > 2
   792 #define FOREACH_PRESENT_L1E(_sl1mfn, _sl1e, _gl1p, _done,  _code) 
      \
   793 do {                                                              
      \
   794     int __done = 0;                                               
      \
   795     _FOREACH_PRESENT_L1E(_sl1mfn, _sl1e, _gl1p,                   
      \
           
<~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   796                          ({ (__done = _done); }), _code);         
      \
rather than at the level of the if, I think

--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253