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


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