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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.06.17 at 11:24, <tim@xxxxxxx> wrote:
> At 03:18 -0600 on 23 Jun (1498187924), Jan Beulich wrote:
>> >>> On 23.06.17 at 10:55, <julien.grall@xxxxxxx> wrote:
>> 
>> > 
>> > 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.
>> 
>> 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

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