| Message ID | 20170619165753.25049-2-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Clean-up memory subsystems | expand |
>>> 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
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.
>>> 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
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.
>>> 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
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 >
>>> 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
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,
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,
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.
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,
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.
>>> 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
>>> 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
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.
>>> 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
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 --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 */
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(-)