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
