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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2026-02-18 13:42, Jan Beulich wrote:
> 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.
Well, since another alternative preprocessor may behave differently or 
even choke on this construct (on any instance!) my guess would be to 
regard this as a GNU extension.
FWIW MISRA disallows this completely because it can lead to UB 87 from 
C99: "There are sequences of preprocessing tokens within the list of 
macro arguments that would otherwise act as preprocessing directives 
(6.10.3)."
So it just sidesteps the issue without having to look at the actual 
token being formed and make our lives as tool implementors a tad easier.
Perhaps a reference to the GCC preprocessor docs could be added in the 
commit message or in the code, just to save some brain cycles again next 
time.
> > > 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
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253