Re: [PATCH] x86/shadow: don't use #if in macro invocations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.02.2026 12:30, Andrew Cooper wrote:
> On 18/02/2026 9:03 am, Jan Beulich wrote:
>> As per the standard this is UB, i.e. we're building on a defacto extension
>> in the compilers we use.
> 
> Is it a real extension, or just something that happens to work?

I was hoping I would not need to go through that large swath of gcc doc to
actually figure, because ...

>>  Misra C:2012 rule 20.6 disallows this altogether,
>> though.

... this I assumed was reason enough. Still, now that you forced me to: In
The C Preprocessor the behavior is described as intentional, but not as an
extension (section "Directives Within Macro Arguments"). Now you get to
judge whether that's a "real" extension or a "de-facto" one.

>> Use helper always-inline functions instead.
>>
>> In sh_audit_l1_table(), along with reducing the scope of "gfn", which now
>> isn't used anymore by the if() side of the conditional, also reduce the
>> scope of two other adjacent variables.
>>
>> For audit_magic() note that both which parameters are needed and what
>> their types are is attributed to AUDIT_FAIL() accessing variables which
>> aren't passed as arguments to it.
> 
> This is grammatically awkward.  IMO it would be clearer to say "For
> audit_magic() note that there are more parameters than might seem
> necessary, caused by the expectations of AUDIT_FAIL()." 

I've switched to using that, but one aspect is lost this way: I would have
preferred both gl1e and sl1e to be plain entries, not pointers to ones.

>> ---
>> Leaving even the fetching of current to the helper in
>> sh_rm_write_access_from_l1() looks tidier to me overall, albeit this means
>> the fetch will now occur once per present L1E.
> 
> This will not make a dent in the performance of the shadow code.
> 
>> Converting the #if to if() and #ifdef to if(IS_ENABLED()) wouldn't work
>> here, as identifiers are used which aren't available when the respective
>> conditions are false.
> 
> Personally, I'd have put this in the main commit message, because it's
> the justification for why out-of-line static inline's need to be used.

I was wondering, so I've moved this up.

>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -395,7 +395,7 @@ static inline mfn_t cf_check sh_next_pag
>>      shadow_set_l2e(d, sl2e, new_sl2e, sl2mfn, SH_type_fl1_shadow, 
>> sh_next_page)
>>  
>>  static inline u32
>> -guest_index(void *ptr)
>> +guest_index(const void *ptr)
>>  {
>>      return (u32)((unsigned long)ptr & ~PAGE_MASK) / sizeof(guest_l1e_t);
>>  }
> 
> While fine per say, this doesn't appear to be related to the patch?

It does, the compiler told me to: type_from_gl3e() uses it, and I really
want to keep the const-s on both of its parameters.

Jan