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

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

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

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