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 10:41, <julien.grall@xxxxxxx> wrote:
> On 23/06/17 09:30, Jan Beulich wrote:
>>>>> On 22.06.17 at 20:31, <julien.grall@xxxxxxx> wrote:
>>> 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.

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


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