Re: Xen reliance on non-standard GCC features

[Michal Orzel <michal.orzel@xxxxxxx>]

On 09/06/2023 10: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.
Apart from vreg.h the same applies to TLB_HELPER of arm32/arm64.
I think also TYPE_SAFE would want to be fixed.

~Michal