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

[Tim Deegan <tim@xxxxxxx>]

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@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 ...
> 
> 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.



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