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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/12/2025 12:28 pm, Nicola Vetrini wrote:
> 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:

Ah, and this adjustment wont fix the violation either.

> 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
>

I hate these constructs with a passion, and from Matrix there's a
separate violation which has no viable fix with the construct staying
like this.

I experimented turning them into syntactically correct foreach_$FOO ( )
loops.  I gave up because it got unwieldy, but now it's the only way I
can see to fix the violation, so I guess I should try again.

I'll drop this one hunk from the patch and commit the rest of the fixes.

~Andrew