xen/mm: Remove claim that INVALID_{MFN,GFN}_INITIALIZER is for older toolchains

[PATCH] xen/mm: Remove claim that INVALID_{MFN,GFN}_INITIALIZER is for older toolchains

Posted by Andrew Cooper 5 days, 12 hours ago

This was never because of a bug in GCC.

C requires that static objects are initialised with constant expressions;
_mfn(), as a static inline, is not and cannot be made to be.

Correct the comments.  No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>

Slightly RFC.  I left 'global variable' alone in the comment, because C's
"object with static storage durations" also isn't ideal; there's one user
which is non-static in terms of visibility.  I'm open to adjusting if we can
figure out some better wording.

In C++, we'd just make _mfn() be constexpr.  It turns out that C23 added this
keyword but restricted it to objects and therefore useless.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3018.htm literally admits
that constexpr on objects exists only to force some diagnostics which were
previously optional.
---
 xen/include/xen/mm-frame.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/include/xen/mm-frame.h b/xen/include/xen/mm-frame.h
index d973aec901fa..80885415a78a 100644
--- a/xen/include/xen/mm-frame.h
+++ b/xen/include/xen/mm-frame.h
@@ -9,8 +9,7 @@ TYPE_SAFE(unsigned long, mfn);
 #define INVALID_MFN_RAW  (~0UL)
 #define INVALID_MFN      _mfn(INVALID_MFN_RAW)
 /*
- * To be used for global variable initialization. This workaround a bug
- * in GCC < 5.0.
+ * To be used for global variable initialization.
  */
 #define INVALID_MFN_INITIALIZER { INVALID_MFN_RAW }
 
@@ -45,8 +44,7 @@ TYPE_SAFE(unsigned long, gfn);
 #define INVALID_GFN_RAW  (~0UL)
 #define INVALID_GFN      _gfn(INVALID_GFN_RAW)
 /*
- * To be used for global variable initialization. This workaround a bug
- * in GCC < 5.0 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856
+ * To be used for global variable initialization.
  */
 #define INVALID_GFN_INITIALIZER { INVALID_GFN_RAW }
 
-- 
2.39.5

Re: [PATCH] xen/mm: Remove claim that INVALID_{MFN,GFN}_INITIALIZER is for older toolchains

Posted by Jan Beulich 5 days, 10 hours ago

On 25.02.2026 13:59, Andrew Cooper wrote:
> This was never because of a bug in GCC.
> 
> C requires that static objects are initialised with constant expressions;
> _mfn(), as a static inline, is not and cannot be made to be.

Of course, and I think the comments were meant differently. What wasn't possible
to use (with -std=gnu99) due to the referenced bug is apparently

#define INVALID_GFN ((gfn_t){ ~0UL })

Now that gcc5 is our baseline, do we perhaps want to use that and do away with
INVALID_GFN_INITIALIZER?

Jan

Re: [PATCH] xen/mm: Remove claim that INVALID_{MFN,GFN}_INITIALIZER is for older toolchains

Posted by Andrew Cooper 5 days, 8 hours ago

On 25/02/2026 2:34 pm, Jan Beulich wrote:
> On 25.02.2026 13:59, Andrew Cooper wrote:
>> This was never because of a bug in GCC.
>>
>> C requires that static objects are initialised with constant expressions;
>> _mfn(), as a static inline, is not and cannot be made to be.
> Of course, and I think the comments were meant differently. What wasn't possible
> to use (with -std=gnu99) due to the referenced bug is apparently
>
> #define INVALID_GFN ((gfn_t){ ~0UL })
>
> Now that gcc5 is our baseline, do we perhaps want to use that and do away with
> INVALID_GFN_INITIALIZER?

Oh.  Yeah that's very much not what the comment suggested.

Changing like that almost works, but there's one snag.  common/memory.c has

    BUILD_BUG_ON(INVALID_GFN_RAW + 1);

and with the _RAW constant wanting to go, the obvious:

    BUILD_BUG_ON(gfn_x(INVALID_GFN) + 1);

doesn't compile as it's no longer a constant expression.

It's not clear what to do here.  I don't think we want to keep
INVALID_GFN_RAW around for just this, but nor am I completely happy
dropping the BUILD_BUG_ON() either.

Thoughts?

~Andrew

Re: [PATCH] xen/mm: Remove claim that INVALID_{MFN,GFN}_INITIALIZER is for older toolchains

Posted by Jan Beulich 4 days, 17 hours ago

On 25.02.2026 17:35, Andrew Cooper wrote:
> On 25/02/2026 2:34 pm, Jan Beulich wrote:
>> On 25.02.2026 13:59, Andrew Cooper wrote:
>>> This was never because of a bug in GCC.
>>>
>>> C requires that static objects are initialised with constant expressions;
>>> _mfn(), as a static inline, is not and cannot be made to be.
>> Of course, and I think the comments were meant differently. What wasn't possible
>> to use (with -std=gnu99) due to the referenced bug is apparently
>>
>> #define INVALID_GFN ((gfn_t){ ~0UL })
>>
>> Now that gcc5 is our baseline, do we perhaps want to use that and do away with
>> INVALID_GFN_INITIALIZER?
> 
> Oh.  Yeah that's very much not what the comment suggested.
> 
> Changing like that almost works, but there's one snag.  common/memory.c has
> 
>     BUILD_BUG_ON(INVALID_GFN_RAW + 1);
> 
> and with the _RAW constant wanting to go, the obvious:
> 
>     BUILD_BUG_ON(gfn_x(INVALID_GFN) + 1);
> 
> doesn't compile as it's no longer a constant expression.
> 
> It's not clear what to do here.  I don't think we want to keep
> INVALID_GFN_RAW around for just this, but nor am I completely happy
> dropping the BUILD_BUG_ON() either.

One option may be to have separate forms for release and debug builds,
with the debug one open-coding gfn_x. Except that this doesn't work: In

    BUILD_BUG_ON(INVALID_GFN._gfn + 1);

the expression is a constant-expression, but not an integer constant
expression.

Hence the next "best" thing I can think of is

    if ( gfn_x(INVALID_GFN) + 1 )
        BUILD_ERROR("bad INVALID_GFN");

It's not quite clear to me whether it would be worthwhile to abstract
this further, e.g. by introducing BUILD_{ERROR,BUG}_IF(). If so,
perhaps we would want to spell out somewhere that BUILD_BUG_ON() is to
be preferred whenever it's usable.

Jan