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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@citrix.com>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>>> CC: consulting@bugseng.com <consulting@bugseng.com>
>>>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>> ---
>>>>>  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