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 <[email protected]>
>>>>>> ---
>>>>>> CC: Jan Beulich <[email protected]>
>>>>>> CC: Roger Pau Monné <[email protected]>
>>>>>> CC: Stefano Stabellini <[email protected]>
>>>>>> CC: [email protected] <[email protected]>
>>>>>> CC: Nicola Vetrini <[email protected]>
>>>>>> ---
>>>>>> 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
