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

[Julien Grall <julien.grall@xxxxxxx>]

On 23/06/17 09:30, Jan Beulich wrote:
> > > > On 22.06.17 at 20:31, <julien.grall@xxxxxxx> wrote:
> > Hi,
> >
> > On 20/06/17 11:32, Jan Beulich wrote:
> > > > > > On 20.06.17 at 12:06, <tim@xxxxxxx> wrote:
> > > > At 03:36 -0600 on 20 Jun (1497929778), Jan Beulich wrote:
> > > > > > > > On 20.06.17 at 11:14, <tim@xxxxxxx> wrote:
> > > > > > At 01:32 -0600 on 20 Jun (1497922345), Jan Beulich wrote:
> > > > > > > > > > On 19.06.17 at 18:57, <julien.grall@xxxxxxx> 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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel