Re: Xen reliance on non-standard GCC features

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 9 Jun 2023, at 15:19, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Jan,
> 
> On 09/06/2023 09:54, Jan Beulich wrote:
>> On 08.06.2023 14:18, Roberto Bagnara wrote:
>>> On 07/06/23 09:39, Jan Beulich wrote:
>>>> On 05.06.2023 15:26, Roberto Bagnara wrote:
>>>>> On 05/06/23 11:28, Jan Beulich wrote:
>>>>>> On 05.06.2023 07:28, Roberto Bagnara wrote:
>>>>> You are right: here are a few examples for U2:
>>>>> 
>>>>> xen/arch/arm/cpuerrata.c:92.12-92.35:
>>>>> empty initializer list (ill-formed for the C99 standard, ISO/IEC 
>>>>> 9899:1999 Section 6.7.8: "An empty initialization list." [STD.emptinit]). 
>>>>> Tool used is `/usr/bin/aarch64-linux-gnu-gcc-12'
>>>>> xen/include/xen/spinlock.h:31.21-31.23: expanded from macro `_LOCK_DEBUG'
>>>>> xen/include/xen/spinlock.h:143.57-143.67: expanded from macro 
>>>>> `SPIN_LOCK_UNLOCKED'
>>>>> xen/include/xen/spinlock.h:144.43-144.60: expanded from macro 
>>>>> `DEFINE_SPINLOCK'
>>>> 
>>>> I'm afraid this is a bad example, as it goes hand-in-hand with using
>>>> another extension. I don't think using a non-empty initialization list
>>>> is going to work with
>>>> 
>>>> union lock_debug { };
>>> 
>>> Yes, this is C99 undefined behavior 58:
>>> "A structure or union is defined as containing no named members (6.7.2.1)."
>>> 
>>> Here is another example:
>>> 
>>> lpae_t pte = {};
>>> 
>>> whereas we have
>>> 
>>> typedef union {
>>>      uint64_t bits;
>>>      lpae_pt_t pt;
>>>      lpae_p2m_t p2m;
>>>      lpae_walk_t walk;
>>> } lpae_t;
>>> 
>>> 
>>>>> xen/arch/arm/cpuerrata.c:678.5-678.6:
>>>>> empty initializer list (ill-formed for the C99 standard, ISO/IEC 
>>>>> 9899:1999 Section 6.7.8: "An empty initialization list." [STD.emptinit]). 
>>>>> Tool used is `/usr/bin/aarch64-linux-gnu-gcc-12'
>>>>> 
>>>>> xen/arch/arm/cpufeature.c:33.5-33.6:
>>>>> empty initializer list (ill-formed for the C99 standard, ISO/IEC 
>>>>> 9899:1999 Section 6.7.8: "An empty initialization list." [STD.emptinit]). 
>>>>> Tool used is `/usr/bin/aarch64-linux-gnu-gcc-12'
>>>> 
>>>> Both of these are a common idiom we use: The "sentinel" of an array
>>>> of compound type initializer.
>>> 
>>> Wouldn't it be possible writing such sentinels in a standard-compliant
>>> way, like {0} or similar, instead of {}?
>> I would be possible, sure, but the question is whether we want that. Iirc
>> in review comments we've been asking to preferably use {}, for being
>> shorter / less clutter without resulting in any ambiguity.
>>>>>>> U6) Empty declarations.
>>>>> 
>>>>> Examples:
>>>>> 
>>>>> xen/arch/arm/arm64/lib/find_next_bit.c:57.29:
>>>>> empty declaration (ill-formed for the C99 standard, ISO/IEC 9899:1999 
>>>>> Section 6.7: "An empty declaration." [STD.emptdecl]). Tool used is 
>>>>> `/usr/bin/aarch64-linux-gnu-gcc-12'
>>>>> 
>>>>> xen/arch/arm/arm64/lib/find_next_bit.c:103.34:
>>>>> empty declaration (ill-formed for the C99 standard, ISO/IEC 9899:1999 
>>>>> Section 6.7: "An empty declaration." [STD.emptdecl]). Tool used is 
>>>>> `/usr/bin/aarch64-linux-gnu-gcc-12'
>>>> 
>>>> Looks like these could be taken care of by finally purging our
>>>> EXPORT_SYMBOL() stub.
>>>> 
>>>>> xen/arch/arm/include/asm/vreg.h:143.26:
>>>>> empty declaration (ill-formed for the C99 standard, ISO/IEC 9899:1999 
>>>>> Section 6.7: "An empty declaration." [STD.emptdecl]). Tool used is 
>>>>> `/usr/bin/aarch64-linux-gnu-gcc-12'
>>>>> 
>>>>> xen/arch/arm/include/asm/vreg.h:144.26:
>>>>> empty declaration (ill-formed for the C99 standard, ISO/IEC 9899:1999 
>>>>> Section 6.7: "An empty declaration." [STD.emptdecl]). Tool used is 
>>>>> `/usr/bin/aarch64-linux-gnu-gcc-12'
>>>> 
>>>> I'm having trouble spotting anything suspicious there.
>>> 
>>> The macro expands to definitions of inline functions
>>> and after the macro invocation there is a ";".
>>> 
>>> The preprocessed code is then:
>>> 
>>> static inline void foo() { ... }
>>> ;
>>> 
>>> where the final ";" is an empty declaration not allowed by
>>> the C99 language standard.
>> Oh, I see.
>>> Removing the ";" after the macro invocation is a possible solution,
>>> but other possibilities exist if this is strongly unwanted.
>> We have other macros to instantiate functions, and there no stray
>> semicolons are used. I think this wants doing the same way here, but it
>> being Arm code the ultimate say is with the Arm maintainers.
> 
> I don't think there is a reason to keep the ";" after. So I would be fine if 
> this is removed.

+1

Cheers
Bertrand