[Xen-devel,01/24] xen/mm: Don't use _{g, m}fn for defining INVALID_{G, M}FN

Message ID 20170613161323.25196-2-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: Extend the usage of typesafe MFN
Related show

Commit Message

Julien Grall June 13, 2017, 4:13 p.m.
INVALID_{G,M}FN are defined using static inline helpers _{g,m}fn.
This means, they cannot be used to initialize a build time static variable:

In file included from mm.c:24:0:
xen/xen/include/xen/mm.h:59:26: error: initializer element is not constant
 #define INVALID_MFN      _mfn(~0UL)

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

I know that this solution will not work for non-debug build. I would
like input from the community on way to fix it nicely.
---
 xen/include/xen/mm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andrew Cooper June 13, 2017, 4:20 p.m. | #1
On 13/06/17 17:13, Julien Grall wrote:
> INVALID_{G,M}FN are defined using static inline helpers _{g,m}fn.
> This means, they cannot be used to initialize a build time static variable:
>
> In file included from mm.c:24:0:
> xen/xen/include/xen/mm.h:59:26: error: initializer element is not constant
>  #define INVALID_MFN      _mfn(~0UL)
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
>
> I know that this solution will not work for non-debug build. I would
> like input from the community on way to fix it nicely.

Hmm - a proper typedef gets inserted.  I presume the compiler objects to
(unsigned long){ ~0UL } to initialise a scalar?

It might be better to move the definition of INVALID_$FOO into the
TYPE_SAFE() declaration so we can create an appropriate initialiser for
each both builds.

~Andrew
Jan Beulich June 14, 2017, 8:40 a.m. | #2
>>> On 13.06.17 at 18:20, <andrew.cooper3@citrix.com> wrote:
> On 13/06/17 17:13, Julien Grall wrote:
>> INVALID_{G,M}FN are defined using static inline helpers _{g,m}fn.
>> This means, they cannot be used to initialize a build time static variable:
>>
>> In file included from mm.c:24:0:
>> xen/xen/include/xen/mm.h:59:26: error: initializer element is not constant
>>  #define INVALID_MFN      _mfn(~0UL)
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>>
>> I know that this solution will not work for non-debug build. I would
>> like input from the community on way to fix it nicely.
> 
> Hmm - a proper typedef gets inserted.  I presume the compiler objects to
> (unsigned long){ ~0UL } to initialise a scalar?
> 
> It might be better to move the definition of INVALID_$FOO into the
> TYPE_SAFE() declaration so we can create an appropriate initialiser for
> each both builds.

Except that you can't put a #define in a macro definition, and producing
a static const wouldn't help Julien's case of wanting it in the initializer of
a static variable. So best I can come up with right now would be to
introduce a separate

#define TYPE_SAFE_CONSTANT(name, val) \
    (name##_t){ val }

and

#define TYPE_SAFE_CONSTANT(name, val) \
    (name##_t)(val)

for the !NDEBUG / NDEBUG cases respectively and use it as

#define INVALID_MFN TYPE_SAFE_CONSTANT(mfn, ~0UL)

Jan
Tim Deegan June 14, 2017, 8:49 a.m. | #3
At 17:13 +0100 on 13 Jun (1497373980), Julien Grall wrote:
> INVALID_{G,M}FN are defined using static inline helpers _{g,m}fn.
> This means, they cannot be used to initialize a build time static variable:
> 
> In file included from mm.c:24:0:
> xen/xen/include/xen/mm.h:59:26: error: initializer element is not constant
>  #define INVALID_MFN      _mfn(~0UL)
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Tim Deegan <tim@xen.org>

> I know that this solution will not work for non-debug build. I would
> like input from the community on way to fix it nicely.

It seems to WFM: https://godbolt.org/g/vEVNY3

Tim.
Julien Grall June 14, 2017, 9:40 a.m. | #4
Hi Tim,

On 06/14/2017 09:49 AM, Tim Deegan wrote:
> At 17:13 +0100 on 13 Jun (1497373980), Julien Grall wrote:
>> INVALID_{G,M}FN are defined using static inline helpers _{g,m}fn.
>> This means, they cannot be used to initialize a build time static variable:
>>
>> In file included from mm.c:24:0:
>> xen/xen/include/xen/mm.h:59:26: error: initializer element is not constant
>>   #define INVALID_MFN      _mfn(~0UL)
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Acked-by: Tim Deegan <tim@xen.org>
> 
>> I know that this solution will not work for non-debug build. I would
>> like input from the community on way to fix it nicely.
> 
> It seems to WFM: https://godbolt.org/g/vEVNY3

To be honest, I thought it would not work and a bit surprised that it 
actually works.

I am happy to keep like that or introduced a new define (such as 
TYPE_SAFE_CONSTANT suggested by Jan).

Cheers,
Jan Beulich June 14, 2017, 9:56 a.m. | #5
>>> On 14.06.17 at 11:40, <julien.grall@arm.com> wrote:
> On 06/14/2017 09:49 AM, Tim Deegan wrote:
>> At 17:13 +0100 on 13 Jun (1497373980), Julien Grall wrote:
>>> INVALID_{G,M}FN are defined using static inline helpers _{g,m}fn.
>>> This means, they cannot be used to initialize a build time static variable:
>>>
>>> In file included from mm.c:24:0:
>>> xen/xen/include/xen/mm.h:59:26: error: initializer element is not constant
>>>   #define INVALID_MFN      _mfn(~0UL)
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> 
>> Acked-by: Tim Deegan <tim@xen.org>
>> 
>>> I know that this solution will not work for non-debug build. I would
>>> like input from the community on way to fix it nicely.
>> 
>> It seems to WFM: https://godbolt.org/g/vEVNY3 
> 
> To be honest, I thought it would not work and a bit surprised that it 
> actually works.

Me too - it didn't occur to me that the extension "Compound Literals"
also, contrary to its name, covers scalar types. But it's indeed
documented, so we don't need to be afraid of it going away.
Remains the question whether our oldest supported gcc allows it.
The oldest one I can easily try (4.3.something) works fine.

Jan

Patch hide | download patch | download mbox

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index ee50d4cd7b..f0daae6b5c 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -56,7 +56,7 @@ 
 
 TYPE_SAFE(unsigned long, mfn);
 #define PRI_mfn          "05lx"
-#define INVALID_MFN      _mfn(~0UL)
+#define INVALID_MFN      (mfn_t){ ~0UL }
 
 #ifndef mfn_t
 #define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
@@ -85,7 +85,7 @@  static inline bool_t mfn_eq(mfn_t x, mfn_t y)
 
 TYPE_SAFE(unsigned long, gfn);
 #define PRI_gfn          "05lx"
-#define INVALID_GFN      _gfn(~0UL)
+#define INVALID_GFN      (gfn_t){ ~0UL }
 
 #ifndef gfn_t
 #define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */