diff mbox

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

Message ID 20170619165753.25049-2-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Julien Grall June 19, 2017, 4:57 p.m. UTC
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>
---
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>

    Changes in v2:
        - Add Tim's acked
        - The solution suggested in v1 is also working for non-debug
        build. So keep the code as it is.
---
 xen/include/xen/mm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich June 20, 2017, 7:32 a.m. UTC | #1
>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
> --- 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 }

While I don't expect anyone to wish to use a suffix expression on
this constant, for maximum compatibility this should still be fully
parenthesized, I think. Of course this should be easy enough to
do while committing.

Are you able to assure us that clang supports this gcc extension
(compound literal for non-compound types), or are we going to
have to see whether clang complains after having committed the
change (which in turn would likely only be found later, once
someone tries a non-debug build with clang)?

Jan
Tim Deegan June 20, 2017, 9:14 a.m. UTC | #2
At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
> >>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
> > --- 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 }
> 
> While I don't expect anyone to wish to use a suffix expression on
> this constant, for maximum compatibility this should still be fully
> parenthesized, I think. Of course this should be easy enough to
> do while committing.
> 
> Are you able to assure us that clang supports this gcc extension
> (compound literal for non-compound types)

AIUI this is a C99 feature, not a GCCism.  Clang supports it as far
back as 3.0: https://godbolt.org/g/YY97uj

Tim.
Jan Beulich June 20, 2017, 9:36 a.m. UTC | #3
>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>> >>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>> > --- 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 }
>> 
>> While I don't expect anyone to wish to use a suffix expression on
>> this constant, for maximum compatibility this should still be fully
>> parenthesized, I think. Of course this should be easy enough to
>> do while committing.
>> 
>> Are you able to assure us that clang supports this gcc extension
>> (compound literal for non-compound types)
> 
> AIUI this is a C99 feature, not a GCCism.

Most parts of it yes (it is a gcc extension in C89 mode only), but the
specific use here isn't afaict: Compound literals outside of functions
are static objects, and hence couldn't be used as initializers of other
objects.

>  Clang supports it as far back as 3.0: https://godbolt.org/g/YY97uj 

Good.

Jan
Tim Deegan June 20, 2017, 10:06 a.m. UTC | #4
At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
> >>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
> > At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
> >> >>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
> >> > --- 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 }
> >> 
> >> While I don't expect anyone to wish to use a suffix expression on
> >> this constant, for maximum compatibility this should still be fully
> >> parenthesized, I think. Of course this should be easy enough to
> >> do while committing.
> >> 
> >> Are you able to assure us that clang supports this gcc extension
> >> (compound literal for non-compound types)
> > 
> > AIUI this is a C99 feature, not a GCCism.
> 
> Most parts of it yes (it is a gcc extension in C89 mode only), but the
> specific use here isn't afaict: Compound literals outside of functions
> are static objects, and hence couldn't be used as initializers of other
> objects.

Ah, I see.  So would it be better to use

  #define INVALID_MFN ((const mfn_t) { ~0UL })

?

Tim.
Jan Beulich June 20, 2017, 10:32 a.m. UTC | #5
>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>> >>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>> > At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>> >> >>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>> >> > --- 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 }
>> >> 
>> >> While I don't expect anyone to wish to use a suffix expression on
>> >> this constant, for maximum compatibility this should still be fully
>> >> parenthesized, I think. Of course this should be easy enough to
>> >> do while committing.
>> >> 
>> >> Are you able to assure us that clang supports this gcc extension
>> >> (compound literal for non-compound types)
>> > 
>> > AIUI this is a C99 feature, not a GCCism.
>> 
>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>> specific use here isn't afaict: Compound literals outside of functions
>> are static objects, and hence couldn't be used as initializers of other
>> objects.
> 
> Ah, I see.  So would it be better to use
> 
>   #define INVALID_MFN ((const mfn_t) { ~0UL })
> 
> ?

While I think we should indeed consider adding the const, the above
still is a static object, and hence still not suitable as an initializer as
per C99 or C11. But as long as gcc and clang permit it, we're fine.

Jan
Julien Grall June 22, 2017, 6:31 p.m. UTC | #6
Hi,

On 20/06/17 11:32, Jan Beulich wrote:
>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>>>>>> --- 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 }
>>>>>
>>>>> While I don't expect anyone to wish to use a suffix expression on
>>>>> this constant, for maximum compatibility this should still be fully
>>>>> parenthesized, I think. Of course this should be easy enough to
>>>>> do while committing.
>>>>>
>>>>> Are you able to assure us that clang supports this gcc extension
>>>>> (compound literal for non-compound types)
>>>>
>>>> AIUI this is a C99 feature, not a GCCism.
>>>
>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>>> specific use here isn't afaict: Compound literals outside of functions
>>> are static objects, and hence couldn't be used as initializers of other
>>> objects.
>>
>> Ah, I see.  So would it be better to use
>>
>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
>>
>> ?
>
> While I think we should indeed consider adding the const, the above
> still is a static object, and hence still not suitable as an initializer as
> per C99 or C11. But as long as gcc and clang permit it, we're fine.

Actually this solutions breaks on GCC 4.9 provided by Linaro ([1] 
4.9-2016-02 and 4.9-2017.01).

This small reproducer does not compile with -std=gnu99 (used by Xen) but 
compile with this option. Jan, have you tried 4.9 with this patch?

typedef struct
{
     unsigned long i;
} mfn_t;

mfn_t v = (const mfn_t){~0UL};

Cheers,

[1] https://releases.linaro.org/components/toolchain/binaries/

>
> Jan
>
Jan Beulich June 23, 2017, 8:30 a.m. UTC | #7
>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
> Hi,
> 
> On 20/06/17 11:32, Jan Beulich wrote:
>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>>>>>>> --- 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 }
>>>>>>
>>>>>> While I don't expect anyone to wish to use a suffix expression on
>>>>>> this constant, for maximum compatibility this should still be fully
>>>>>> parenthesized, I think. Of course this should be easy enough to
>>>>>> do while committing.
>>>>>>
>>>>>> Are you able to assure us that clang supports this gcc extension
>>>>>> (compound literal for non-compound types)
>>>>>
>>>>> AIUI this is a C99 feature, not a GCCism.
>>>>
>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>>>> specific use here isn't afaict: Compound literals outside of functions
>>>> are static objects, and hence couldn't be used as initializers of other
>>>> objects.
>>>
>>> Ah, I see.  So would it be better to use
>>>
>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
>>>
>>> ?
>>
>> While I think we should indeed consider adding the const, the above
>> still is a static object, and hence still not suitable as an initializer as
>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
> 
> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1] 
> 4.9-2016-02 and 4.9-2017.01).
> 
> This small reproducer does not compile with -std=gnu99 (used by Xen) but 
> compile with this option. Jan, have you tried 4.9 with this patch?

That's sort of an odd question - you've sent the patch, so I would
have expected you to have made sure it doesn't break (and
while it was me to add the const, this was discussed, and you don't
make clear whether that's the issue). In any event, I've tried ...

> typedef struct
> {
>      unsigned long i;
> } mfn_t;
> 
> mfn_t v = (const mfn_t){~0UL};

... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
of them compile this without errors or warnings (at -Wall -W).
For 4.9.3 I've also specifically taken care to try not only the
x86 compiler, but also the arm32 and arm64 ones. So I'm afraid
I lack enough detail to understand what the issue is and what a
solution may look like.

Jan
Julien Grall June 23, 2017, 8:41 a.m. UTC | #8
Hi Jan,

On 23/06/17 09:30, Jan Beulich wrote:
>>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
>> Hi,
>>
>> On 20/06/17 11:32, Jan Beulich wrote:
>>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
>>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>>>>>>>> --- 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 }
>>>>>>>
>>>>>>> While I don't expect anyone to wish to use a suffix expression on
>>>>>>> this constant, for maximum compatibility this should still be fully
>>>>>>> parenthesized, I think. Of course this should be easy enough to
>>>>>>> do while committing.
>>>>>>>
>>>>>>> Are you able to assure us that clang supports this gcc extension
>>>>>>> (compound literal for non-compound types)
>>>>>>
>>>>>> AIUI this is a C99 feature, not a GCCism.
>>>>>
>>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>>>>> specific use here isn't afaict: Compound literals outside of functions
>>>>> are static objects, and hence couldn't be used as initializers of other
>>>>> objects.
>>>>
>>>> Ah, I see.  So would it be better to use
>>>>
>>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
>>>>
>>>> ?
>>>
>>> While I think we should indeed consider adding the const, the above
>>> still is a static object, and hence still not suitable as an initializer as
>>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
>>
>> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
>> 4.9-2016-02 and 4.9-2017.01).
>>
>> This small reproducer does not compile with -std=gnu99 (used by Xen) but
>> compile with this option. Jan, have you tried 4.9 with this patch?
>
> That's sort of an odd question - you've sent the patch, so I would
> have expected you to have made sure it doesn't break (and
> while it was me to add the const, this was discussed, and you don't
> make clear whether that's the issue). In any event, I've tried ...

I don't personally try every single compiler every time I am writing a 
patch... This is too complex given that different stakeholders (Linaro, 
Debian, Ubuntu,...) provide various binaries with their own patches on top.

I asked you because I was wondering what is happening on x86 (I don't 
have 4.9 x86 in hand) and to rule out a bug in the compiler provided by 
Linaro.

>
>> typedef struct
>> {
>>      unsigned long i;
>> } mfn_t;
>>
>> mfn_t v = (const mfn_t){~0UL};
>
> ... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
> of them compile this without errors or warnings (at -Wall -W).
> For 4.9.3 I've also specifically taken care to try not only the
> x86 compiler, but also the arm32 and arm64 ones. So I'm afraid
> I lack enough detail to understand what the issue is and what a
> solution may look like.

I don't have much except the following error:

/tmp/test.c:6:1: error: initializer element is not constant
  mfn_t v = (const mfn_t){~0UL};
  ^

If it works for you on 4.9, then it might be a bug in the GCC provided 
by Linaro and will report it.

Cheers,
Julien Grall June 23, 2017, 8:55 a.m. UTC | #9
On 23/06/17 09:30, Jan Beulich wrote:
>>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
>> Hi,
>>
>> On 20/06/17 11:32, Jan Beulich wrote:
>>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
>>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>>>>>>>> --- 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 }
>>>>>>>
>>>>>>> While I don't expect anyone to wish to use a suffix expression on
>>>>>>> this constant, for maximum compatibility this should still be fully
>>>>>>> parenthesized, I think. Of course this should be easy enough to
>>>>>>> do while committing.
>>>>>>>
>>>>>>> Are you able to assure us that clang supports this gcc extension
>>>>>>> (compound literal for non-compound types)
>>>>>>
>>>>>> AIUI this is a C99 feature, not a GCCism.
>>>>>
>>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>>>>> specific use here isn't afaict: Compound literals outside of functions
>>>>> are static objects, and hence couldn't be used as initializers of other
>>>>> objects.
>>>>
>>>> Ah, I see.  So would it be better to use
>>>>
>>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
>>>>
>>>> ?
>>>
>>> While I think we should indeed consider adding the const, the above
>>> still is a static object, and hence still not suitable as an initializer as
>>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
>>
>> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
>> 4.9-2016-02 and 4.9-2017.01).
>>
>> This small reproducer does not compile with -std=gnu99 (used by Xen) but
>> compile with this option. Jan, have you tried 4.9 with this patch?
>
> That's sort of an odd question - you've sent the patch, so I would
> have expected you to have made sure it doesn't break (and
> while it was me to add the const, this was discussed, and you don't
> make clear whether that's the issue). In any event, I've tried ...
>
>> typedef struct
>> {
>>      unsigned long i;
>> } mfn_t;
>>
>> mfn_t v = (const mfn_t){~0UL};
>
> ... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
> of them compile this without errors or warnings (at -Wall -W).

Actually did you build with -std=gnu99? I just tried 4.9.3 for x86 and
also 4.8 for ARM64 on Ubuntu Trusty. Both are broken.

Cheers,
Tim Deegan June 23, 2017, 8:58 a.m. UTC | #10
At 09:41 +0100 on 23 Jun (1498210893), Julien Grall wrote:
> Hi Jan,
> 
> On 23/06/17 09:30, Jan Beulich wrote:
> >>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
> >> Hi,
> >>
> >> On 20/06/17 11:32, Jan Beulich wrote:
> >>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
> >>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
> >>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
> >>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
> >>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
> >>>>>>>> --- 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 }
> >>>>>>>
> >>>>>>> While I don't expect anyone to wish to use a suffix expression on
> >>>>>>> this constant, for maximum compatibility this should still be fully
> >>>>>>> parenthesized, I think. Of course this should be easy enough to
> >>>>>>> do while committing.
> >>>>>>>
> >>>>>>> Are you able to assure us that clang supports this gcc extension
> >>>>>>> (compound literal for non-compound types)
> >>>>>>
> >>>>>> AIUI this is a C99 feature, not a GCCism.
> >>>>>
> >>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
> >>>>> specific use here isn't afaict: Compound literals outside of functions
> >>>>> are static objects, and hence couldn't be used as initializers of other
> >>>>> objects.
> >>>>
> >>>> Ah, I see.  So would it be better to use
> >>>>
> >>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
> >>>>
> >>>> ?
> >>>
> >>> While I think we should indeed consider adding the const, the above
> >>> still is a static object, and hence still not suitable as an initializer as
> >>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
> >>
> >> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
> >> 4.9-2016-02 and 4.9-2017.01).
> >>
> >> This small reproducer does not compile with -std=gnu99 (used by Xen) but
> >> compile with this option. Jan, have you tried 4.9 with this patch?
> >
> > That's sort of an odd question - you've sent the patch, so I would
> > have expected you to have made sure it doesn't break (and
> > while it was me to add the const, this was discussed, and you don't
> > make clear whether that's the issue). In any event, I've tried ...
> 
> I don't personally try every single compiler every time I am writing a 
> patch... This is too complex given that different stakeholders (Linaro, 
> Debian, Ubuntu,...) provide various binaries with their own patches on top.
> 
> I asked you because I was wondering what is happening on x86 (I don't 
> have 4.9 x86 in hand) and to rule out a bug in the compiler provided by 
> Linaro.
> 
> >
> >> typedef struct
> >> {
> >>      unsigned long i;
> >> } mfn_t;
> >>
> >> mfn_t v = (const mfn_t){~0UL};
> >
> > ... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
> > of them compile this without errors or warnings (at -Wall -W).
> > For 4.9.3 I've also specifically taken care to try not only the
> > x86 compiler, but also the arm32 and arm64 ones. So I'm afraid
> > I lack enough detail to understand what the issue is and what a
> > solution may look like.
> 
> I don't have much except the following error:
> 
> /tmp/test.c:6:1: error: initializer element is not constant
>   mfn_t v = (const mfn_t){~0UL};
>   ^
> 
> If it works for you on 4.9, then it might be a bug in the GCC provided 
> by Linaro and will report it.

This fails for me on x86 gcc 4.9.4, using -xc -std=gnu99.  The
complaint is valid, as Jan pointed out: the literal is a static object
and so not a valid initializer.  GCC also complains about the
'debug' version for the same reason. :(

Using a plain initializer works OK for both debug and non-debug:

  mfn_t v = {~0UL};

but I haven't checked whether other compilers like that as well.

Tim.
Julien Grall June 23, 2017, 9:01 a.m. UTC | #11
On 23/06/17 09:58, Tim Deegan wrote:
> At 09:41 +0100 on 23 Jun (1498210893), Julien Grall wrote:
>> Hi Jan,
>>
>> On 23/06/17 09:30, Jan Beulich wrote:
>>>>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
>>>> Hi,
>>>>
>>>> On 20/06/17 11:32, Jan Beulich wrote:
>>>>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
>>>>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>>>>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>>>>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>>>>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>>>>>>>>>> --- 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 }
>>>>>>>>>
>>>>>>>>> While I don't expect anyone to wish to use a suffix expression on
>>>>>>>>> this constant, for maximum compatibility this should still be fully
>>>>>>>>> parenthesized, I think. Of course this should be easy enough to
>>>>>>>>> do while committing.
>>>>>>>>>
>>>>>>>>> Are you able to assure us that clang supports this gcc extension
>>>>>>>>> (compound literal for non-compound types)
>>>>>>>>
>>>>>>>> AIUI this is a C99 feature, not a GCCism.
>>>>>>>
>>>>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>>>>>>> specific use here isn't afaict: Compound literals outside of functions
>>>>>>> are static objects, and hence couldn't be used as initializers of other
>>>>>>> objects.
>>>>>>
>>>>>> Ah, I see.  So would it be better to use
>>>>>>
>>>>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
>>>>>>
>>>>>> ?
>>>>>
>>>>> While I think we should indeed consider adding the const, the above
>>>>> still is a static object, and hence still not suitable as an initializer as
>>>>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
>>>>
>>>> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
>>>> 4.9-2016-02 and 4.9-2017.01).
>>>>
>>>> This small reproducer does not compile with -std=gnu99 (used by Xen) but
>>>> compile with this option. Jan, have you tried 4.9 with this patch?
>>>
>>> That's sort of an odd question - you've sent the patch, so I would
>>> have expected you to have made sure it doesn't break (and
>>> while it was me to add the const, this was discussed, and you don't
>>> make clear whether that's the issue). In any event, I've tried ...
>>
>> I don't personally try every single compiler every time I am writing a
>> patch... This is too complex given that different stakeholders (Linaro,
>> Debian, Ubuntu,...) provide various binaries with their own patches on top.
>>
>> I asked you because I was wondering what is happening on x86 (I don't
>> have 4.9 x86 in hand) and to rule out a bug in the compiler provided by
>> Linaro.
>>
>>>
>>>> typedef struct
>>>> {
>>>>      unsigned long i;
>>>> } mfn_t;
>>>>
>>>> mfn_t v = (const mfn_t){~0UL};
>>>
>>> ... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
>>> of them compile this without errors or warnings (at -Wall -W).
>>> For 4.9.3 I've also specifically taken care to try not only the
>>> x86 compiler, but also the arm32 and arm64 ones. So I'm afraid
>>> I lack enough detail to understand what the issue is and what a
>>> solution may look like.
>>
>> I don't have much except the following error:
>>
>> /tmp/test.c:6:1: error: initializer element is not constant
>>   mfn_t v = (const mfn_t){~0UL};
>>   ^
>>
>> If it works for you on 4.9, then it might be a bug in the GCC provided
>> by Linaro and will report it.
>
> This fails for me on x86 gcc 4.9.4, using -xc -std=gnu99.  The
> complaint is valid, as Jan pointed out: the literal is a static object
> and so not a valid initializer.  GCC also complains about the
> 'debug' version for the same reason. :(
>
> Using a plain initializer works OK for both debug and non-debug:
>
>   mfn_t v = {~0UL};
>
> but I haven't checked whether other compilers like that as well.

This seems to be a bug in GCC up to 5.0:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856

Cheers,
Tim Deegan June 23, 2017, 9:02 a.m. UTC | #12
At 09:58 +0100 on 23 Jun (1498211910), Tim Deegan wrote:
> At 09:41 +0100 on 23 Jun (1498210893), Julien Grall wrote:
> > On 23/06/17 09:30, Jan Beulich wrote:
> > >
> > >> typedef struct
> > >> {
> > >>      unsigned long i;
> > >> } mfn_t;
> > >>
> > >> mfn_t v = (const mfn_t){~0UL};
> > >
> > > ... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
> > > of them compile this without errors or warnings (at -Wall -W).
> > > For 4.9.3 I've also specifically taken care to try not only the
> > > x86 compiler, but also the arm32 and arm64 ones. So I'm afraid
> > > I lack enough detail to understand what the issue is and what a
> > > solution may look like.
> > 
> > I don't have much except the following error:
> > 
> > /tmp/test.c:6:1: error: initializer element is not constant
> >   mfn_t v = (const mfn_t){~0UL};
> >   ^
> > 
> > If it works for you on 4.9, then it might be a bug in the GCC provided 
> > by Linaro and will report it.
> 
> This fails for me on x86 gcc 4.9.4, using -xc -std=gnu99.  The
> complaint is valid, as Jan pointed out: the literal is a static object
> and so not a valid initializer.  GCC also complains about the
> 'debug' version for the same reason. :(
> 
> Using a plain initializer works OK for both debug and non-debug:
> 
>   mfn_t v = {~0UL};
> 
> but I haven't checked whether other compilers like that as well.

And that wouldn't work for things like f(INVALID_MFN) -- sometimes we
actually do want the literal.

Tim.
Jan Beulich June 23, 2017, 9:09 a.m. UTC | #13
>>> On 23.06.17 at 10:41, <julien.grall@arm.com> wrote:
> On 23/06/17 09:30, Jan Beulich wrote:
>>>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
>>> On 20/06/17 11:32, Jan Beulich wrote:
>>>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
>>>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>>>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>>>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>>>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>>>>>>>>> --- 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 }
>>>>>>>>
>>>>>>>> While I don't expect anyone to wish to use a suffix expression on
>>>>>>>> this constant, for maximum compatibility this should still be fully
>>>>>>>> parenthesized, I think. Of course this should be easy enough to
>>>>>>>> do while committing.
>>>>>>>>
>>>>>>>> Are you able to assure us that clang supports this gcc extension
>>>>>>>> (compound literal for non-compound types)
>>>>>>>
>>>>>>> AIUI this is a C99 feature, not a GCCism.
>>>>>>
>>>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>>>>>> specific use here isn't afaict: Compound literals outside of functions
>>>>>> are static objects, and hence couldn't be used as initializers of other
>>>>>> objects.
>>>>>
>>>>> Ah, I see.  So would it be better to use
>>>>>
>>>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
>>>>>
>>>>> ?
>>>>
>>>> While I think we should indeed consider adding the const, the above
>>>> still is a static object, and hence still not suitable as an initializer as
>>>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
>>>
>>> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
>>> 4.9-2016-02 and 4.9-2017.01).
>>>
>>> This small reproducer does not compile with -std=gnu99 (used by Xen) but
>>> compile with this option. Jan, have you tried 4.9 with this patch?
>>
>> That's sort of an odd question - you've sent the patch, so I would
>> have expected you to have made sure it doesn't break (and
>> while it was me to add the const, this was discussed, and you don't
>> make clear whether that's the issue). In any event, I've tried ...
> 
> I don't personally try every single compiler every time I am writing a 
> patch... This is too complex given that different stakeholders (Linaro, 
> Debian, Ubuntu,...) provide various binaries with their own patches on top.

Which I can understand. I've asked back the way I did just because
you appeared to imply I would do such checking routinely, which
(for the very reasons you name) I don't. I just happen to test with
differing compiler versions every once in a while.

Jan
Jan Beulich June 23, 2017, 9:18 a.m. UTC | #14
>>> On 23.06.17 at 10:55, <julien.grall@arm.com> wrote:

> 
> On 23/06/17 09:30, Jan Beulich wrote:
>>>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
>>> Hi,
>>>
>>> On 20/06/17 11:32, Jan Beulich wrote:
>>>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
>>>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>>>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>>>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>>>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>>>>>>>>> --- 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 }
>>>>>>>>
>>>>>>>> While I don't expect anyone to wish to use a suffix expression on
>>>>>>>> this constant, for maximum compatibility this should still be fully
>>>>>>>> parenthesized, I think. Of course this should be easy enough to
>>>>>>>> do while committing.
>>>>>>>>
>>>>>>>> Are you able to assure us that clang supports this gcc extension
>>>>>>>> (compound literal for non-compound types)
>>>>>>>
>>>>>>> AIUI this is a C99 feature, not a GCCism.
>>>>>>
>>>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>>>>>> specific use here isn't afaict: Compound literals outside of functions
>>>>>> are static objects, and hence couldn't be used as initializers of other
>>>>>> objects.
>>>>>
>>>>> Ah, I see.  So would it be better to use
>>>>>
>>>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
>>>>>
>>>>> ?
>>>>
>>>> While I think we should indeed consider adding the const, the above
>>>> still is a static object, and hence still not suitable as an initializer as
>>>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
>>>
>>> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
>>> 4.9-2016-02 and 4.9-2017.01).
>>>
>>> This small reproducer does not compile with -std=gnu99 (used by Xen) but
>>> compile with this option. Jan, have you tried 4.9 with this patch?
>>
>> That's sort of an odd question - you've sent the patch, so I would
>> have expected you to have made sure it doesn't break (and
>> while it was me to add the const, this was discussed, and you don't
>> make clear whether that's the issue). In any event, I've tried ...
>>
>>> typedef struct
>>> {
>>>      unsigned long i;
>>> } mfn_t;
>>>
>>> mfn_t v = (const mfn_t){~0UL};
>>
>> ... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
>> of them compile this without errors or warnings (at -Wall -W).
> 
> Actually did you build with -std=gnu99? I just tried 4.9.3 for x86 and
> also 4.8 for ARM64 on Ubuntu Trusty. Both are broken.

Ah, indeed - that fails with 4.9.3 but succeeds with 5.2.0. And
it's not the const getting in the way here. I notice this difference
in their documentation (4.9.3 first, then 7.1.0):

Compound literals for scalar types and union types are also allowed,
but then the compound literal is equivalent to a cast.

Compound literals for scalar types and union types are also allowed.
In the following example the variable i is initialized to the value 2,
the result of incrementing the unnamed object created by the
compound literal.

	int i = ++(int) { 1 };

It is especially this example clarifying that newer compilers don't
treat this like a cast anymore (albeit a casted expression alone is
fine as initializer in 4.9.3, so there must be more to the failure).

While I still view this as a compiler bug (as it accepts the code in
default mode), as a workaround  I guess we'll need to accept a
gcc < 5 conditional in the header, which we would really have
wanted to avoid.

Jan
Tim Deegan June 23, 2017, 9:24 a.m. UTC | #15
At 03:18 -0600 on 23 Jun (1498187924), Jan Beulich wrote:
> >>> On 23.06.17 at 10:55, <julien.grall@arm.com> wrote:
> 
> > 
> > On 23/06/17 09:30, Jan Beulich wrote:
> >>>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
> >>> Hi,
> >>>
> >>> On 20/06/17 11:32, Jan Beulich wrote:
> >>>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
> >>>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
> >>>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
> >>>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
> >>>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
> >>>>>>>>> --- 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 }
> >>>>>>>>
> >>>>>>>> While I don't expect anyone to wish to use a suffix expression on
> >>>>>>>> this constant, for maximum compatibility this should still be fully
> >>>>>>>> parenthesized, I think. Of course this should be easy enough to
> >>>>>>>> do while committing.
> >>>>>>>>
> >>>>>>>> Are you able to assure us that clang supports this gcc extension
> >>>>>>>> (compound literal for non-compound types)
> >>>>>>>
> >>>>>>> AIUI this is a C99 feature, not a GCCism.
> >>>>>>
> >>>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
> >>>>>> specific use here isn't afaict: Compound literals outside of functions
> >>>>>> are static objects, and hence couldn't be used as initializers of other
> >>>>>> objects.
> >>>>>
> >>>>> Ah, I see.  So would it be better to use
> >>>>>
> >>>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
> >>>>>
> >>>>> ?
> >>>>
> >>>> While I think we should indeed consider adding the const, the above
> >>>> still is a static object, and hence still not suitable as an initializer as
> >>>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
> >>>
> >>> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
> >>> 4.9-2016-02 and 4.9-2017.01).
> >>>
> >>> This small reproducer does not compile with -std=gnu99 (used by Xen) but
> >>> compile with this option. Jan, have you tried 4.9 with this patch?
> >>
> >> That's sort of an odd question - you've sent the patch, so I would
> >> have expected you to have made sure it doesn't break (and
> >> while it was me to add the const, this was discussed, and you don't
> >> make clear whether that's the issue). In any event, I've tried ...
> >>
> >>> typedef struct
> >>> {
> >>>      unsigned long i;
> >>> } mfn_t;
> >>>
> >>> mfn_t v = (const mfn_t){~0UL};
> >>
> >> ... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
> >> of them compile this without errors or warnings (at -Wall -W).
> > 
> > Actually did you build with -std=gnu99? I just tried 4.9.3 for x86 and
> > also 4.8 for ARM64 on Ubuntu Trusty. Both are broken.
> 
> Ah, indeed - that fails with 4.9.3 but succeeds with 5.2.0. And
> it's not the const getting in the way here. I notice this difference
> in their documentation (4.9.3 first, then 7.1.0):
> 
> Compound literals for scalar types and union types are also allowed,
> but then the compound literal is equivalent to a cast.
> 
> Compound literals for scalar types and union types are also allowed.
> In the following example the variable i is initialized to the value 2,
> the result of incrementing the unnamed object created by the
> compound literal.
> 
> 	int i = ++(int) { 1 };
> 
> It is especially this example clarifying that newer compilers don't
> treat this like a cast anymore (albeit a casted expression alone is
> fine as initializer in 4.9.3, so there must be more to the failure).
> 
> While I still view this as a compiler bug (as it accepts the code in
> default mode), as a workaround  I guess we'll need to accept a
> gcc < 5 conditional in the header, which we would really have
> wanted to avoid.

Since we'll have to make some scheme that works for 4.9, I think we
should just use that for all versions.

How about:
 - keep INVALID_MFN as an inline function call for most uses;
 - #define INVALID_MFN_INITIALIZER { ~0UL } for when we need a
   real constant initializer aat file scope.

Tim.
Jan Beulich June 23, 2017, 9:31 a.m. UTC | #16
>>> On 23.06.17 at 11:24, <tim@xen.org> wrote:
> At 03:18 -0600 on 23 Jun (1498187924), Jan Beulich wrote:
>> >>> On 23.06.17 at 10:55, <julien.grall@arm.com> wrote:
>> 
>> > 
>> > On 23/06/17 09:30, Jan Beulich wrote:
>> >>>>> On 22.06.17 at 20:31, <julien.grall@arm.com> wrote:
>> >>> Hi,
>> >>>
>> >>> On 20/06/17 11:32, Jan Beulich wrote:
>> >>>>>>> On 20.06.17 at 12:06, <tim@xen.org> wrote:
>> >>>>> At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
>> >>>>>>>>> On 20.06.17 at 11:14, <tim@xen.org> wrote:
>> >>>>>>> At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
>> >>>>>>>>>>> On 19.06.17 at 18:57, <julien.grall@arm.com> wrote:
>> >>>>>>>>> --- 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 }
>> >>>>>>>>
>> >>>>>>>> While I don't expect anyone to wish to use a suffix expression on
>> >>>>>>>> this constant, for maximum compatibility this should still be fully
>> >>>>>>>> parenthesized, I think. Of course this should be easy enough to
>> >>>>>>>> do while committing.
>> >>>>>>>>
>> >>>>>>>> Are you able to assure us that clang supports this gcc extension
>> >>>>>>>> (compound literal for non-compound types)
>> >>>>>>>
>> >>>>>>> AIUI this is a C99 feature, not a GCCism.
>> >>>>>>
>> >>>>>> Most parts of it yes (it is a gcc extension in C89 mode only), but the
>> >>>>>> specific use here isn't afaict: Compound literals outside of functions
>> >>>>>> are static objects, and hence couldn't be used as initializers of other
>> >>>>>> objects.
>> >>>>>
>> >>>>> Ah, I see.  So would it be better to use
>> >>>>>
>> >>>>>   #define INVALID_MFN ((const mfn_t) { ~0UL })
>> >>>>>
>> >>>>> ?
>> >>>>
>> >>>> While I think we should indeed consider adding the const, the above
>> >>>> still is a static object, and hence still not suitable as an initializer as
>> >>>> per C99 or C11. But as long as gcc and clang permit it, we're fine.
>> >>>
>> >>> Actually this solutions breaks on GCC 4.9 provided by Linaro ([1]
>> >>> 4.9-2016-02 and 4.9-2017.01).
>> >>>
>> >>> This small reproducer does not compile with -std=gnu99 (used by Xen) but
>> >>> compile with this option. Jan, have you tried 4.9 with this patch?
>> >>
>> >> That's sort of an odd question - you've sent the patch, so I would
>> >> have expected you to have made sure it doesn't break (and
>> >> while it was me to add the const, this was discussed, and you don't
>> >> make clear whether that's the issue). In any event, I've tried ...
>> >>
>> >>> typedef struct
>> >>> {
>> >>>      unsigned long i;
>> >>> } mfn_t;
>> >>>
>> >>> mfn_t v = (const mfn_t){~0UL};
>> >>
>> >> ... this now with 7.1.0, 6.3.0, 5.4.0, 5.2.0, and 4.9.3, and all
>> >> of them compile this without errors or warnings (at -Wall -W).
>> > 
>> > Actually did you build with -std=gnu99? I just tried 4.9.3 for x86 and
>> > also 4.8 for ARM64 on Ubuntu Trusty. Both are broken.
>> 
>> Ah, indeed - that fails with 4.9.3 but succeeds with 5.2.0. And
>> it's not the const getting in the way here. I notice this difference
>> in their documentation (4.9.3 first, then 7.1.0):
>> 
>> Compound literals for scalar types and union types are also allowed,
>> but then the compound literal is equivalent to a cast.
>> 
>> Compound literals for scalar types and union types are also allowed.
>> In the following example the variable i is initialized to the value 2,
>> the result of incrementing the unnamed object created by the
>> compound literal.
>> 
>> 	int i = ++(int) { 1 };
>> 
>> It is especially this example clarifying that newer compilers don't
>> treat this like a cast anymore (albeit a casted expression alone is
>> fine as initializer in 4.9.3, so there must be more to the failure).
>> 
>> While I still view this as a compiler bug (as it accepts the code in
>> default mode), as a workaround  I guess we'll need to accept a
>> gcc < 5 conditional in the header, which we would really have
>> wanted to avoid.
> 
> Since we'll have to make some scheme that works for 4.9, I think we
> should just use that for all versions.
> 
> How about:
>  - keep INVALID_MFN as an inline function call for most uses;
>  - #define INVALID_MFN_INITIALIZER { ~0UL } for when we need a
>    real constant initializer aat file scope.

I'd be fine with that as much as with the other model.

Jan
Julien Grall June 26, 2017, 1:11 p.m. UTC | #17
Hi,

On 23/06/17 10:31, Jan Beulich wrote:
>>>> On 23.06.17 at 11:24, <tim@xen.org> wrote:
>> At 03:18 -0600 on 23 Jun (1498187924), Jan Beulich wrote:
>> How about:
>>  - keep INVALID_MFN as an inline function call for most uses;
>>  - #define INVALID_MFN_INITIALIZER { ~0UL } for when we need a
>>    real constant initializer aat file scope.
>
> I'd be fine with that as much as with the other model.

I will send a patch to revert 725039d39e "mm: don't use _{g,m}fn for 
defining INVALID_{G,M}FN" and one to add INVALID_MFN_INITIALIZER.

Cheers,
diff mbox

Patch

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index c9198232e2..e61b6e991c 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 */
@@ -89,7 +89,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 */