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:
>>> U1) Use of _Static_assert in C99 mode.
>>>
>>> U2) Empty initialization lists, both in C99 mode (ARM64 and X86_64)
>>> and C18 mode (only X86_64).
>>>
>>> U3) Returning void expressions.
>>
>> As per above, tiny extensions like these are, I think, unlikely to be
>> mentioned anywhere explicitly (or more than in passing). For the last of
>> the three it may further be that it pre-dates when gcc started to
>> properly document extensions. Oh, actually - U3 is documented along with
>> -Wreturn-type.
>
> Noted: thanks!
>
>> Uses are generally intentional afaik, but eliminating cases of U2 and U3
>> would likely be possible with just slight overall impact.
>
> Ok. As this has an impact on MISRA compliance at some stage we will need
> an official position on the subject.
>
>> As to U2, it's not clear why you distinguish C99 and C18 mode.
>
> I specified that because for MISRA compliance we need to stick
> to one version of the language: while most translation units
> are compiled in C99 mode, some are compiled in C18 mode and some
> in C90 mode. However, I agree this is OT for the current discussion.
>
>> Throughout
>> this summary of yours it would likely have been helpful if an example was
>> provided for the behavior your describing, when the wording doesn't make
>> it crystal clear (e.g. no example needed for U1 and U3 above).
>
> 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 { };
> 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.
>>> U4) Static functions or variables used in inline functions with external
>>> linkage.
>>
>> There's not a lot of "extern inline" that we've accumulated so far, I
>> think. The only ones I'm aware of are sort() and bsearch(), and there
>> the use is precisely for allowing the compiler to optimize away function
>> calls.
>>
>> The documentation of this functionality is that of the gnu_inline
>> function attribute, afaict. That would be 6.33.1 "Common Function
>> Attributes" in 13.1.0 doc.
>
> No, it is not that one:
>
> xen/common/spinlock.c:316.29-316.40:
> static function `observe_head(spinlock_tickets_t*)' is used in an inline
> function with external linkage (ill-formed for the C99 standard, ISO/IEC
> 9899:1999: "An ill-formed source detected by the parser."
> [STD.diag/ext_internal_in_extern_inline_quiet]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
> xen/common/spinlock.c:301.26-301.37:
> `observe_head(spinlock_tickets_t*)' declared here
> xen/include/xen/spinlock.h:180.1-180.4:
> use 'static' to give inline function `_spin_lock_cb(spinlock_t*,
> void(*)(void*), void*)' internal linkage
Oh, I see. I guess by introducing another wrapper we could deal with
that in sufficiently unintrusive a way.
>>> U5) Enumerator values outside the range of `int'.
>
> Examples:
>
> xen/arch/x86/include/asm/guest/hyperv-tlfs.h:477.9-477.26: Loc #1 [culprit:
> ISO C restricts enumerator values to range of 'int' (2147483648 is too large)
> (ill-formed for the C99 standard, ISO/IEC 9899:1999: "An ill-formed source
> detected by the parser." [STD.diag/ext_enum_value_not_int]). Tool used is
> `/usr/bin/x86_64-linux-gnu-gcc-12']
> xen/arch/x86/include/asm/guest/hyperv-tlfs.h:477.43-477.52: Loc #2 [evidence:
> ISO C restricts enumerator values to range of 'int' (2147483648 is too large)]
>
> xen/arch/x86/include/asm/guest/hyperv-tlfs.h:478.9-478.27: Loc #1 [culprit:
> ISO C restricts enumerator values to range of 'int' (2147483649 is too large)
> (ill-formed for the C99 standard, ISO/IEC 9899:1999: "An ill-formed source
> detected by the parser." [STD.diag/ext_enum_value_not_int]). Tool used is
> `/usr/bin/x86_64-linux-gnu-gcc-12']
> xen/arch/x86/include/asm/guest/hyperv-tlfs.h:478.43-478.52: Loc #2 [evidence:
> ISO C restricts enumerator values to range of 'int' (2147483649 is too large)]
It's not entirely clear to me in how far hyperv-tlfs.h is a header
fully of our own. It may be possible to switch to #define instead if
we don't "follow" someone else's header.
> xen/arch/x86/include/asm/hvm/svm/vmcb.h:143.5-143.27: Loc #1 [culprit: ISO C
> restricts enumerator values to range of 'int' (2147483648 is too large)
> (ill-formed for the C99 standard, ISO/IEC 9899:1999: "An ill-formed source
> detected by the parser." [STD.diag/ext_enum_value_not_int]). Tool used is
> `/usr/bin/x86_64-linux-gnu-gcc-12']
> xen/arch/x86/include/asm/hvm/svm/vmcb.h:143.31-143.38: Loc #2 [evidence: ISO
> C restricts enumerator values to range of 'int' (2147483648 is too large)]
Here we have more leeway, and imo the offending enumerations are an
abuse of "enum" in the first place.
>>> 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.
> xen/arch/arm/include/asm/arm64/flushtlb.h:70.51:
> 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'
Same here. I the tool having an issue with the instantiation of
(inline) functions by way of a macro?
>>> U7) Empty enum definitions.
>
> Example:
>
> xen/arch/arm/include/asm/vgic.h:275.6-275.17:
> enum declaration `gic_sgi_mode' is incomplete (ill-formed for the C99
> standard, ISO/IEC 9899:1999 Section 6.7.2.2: "An incomplete enum
> declaration." [STD.emptenum]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
>
>> ... here I wonder whether instead you mean forward declaration of enums
>> (i.e. what the standard allows for structures and unions only).
>
> It is reported as an enum forward declaration if the enum is later
> properly declared. My understanding is that this is not the case
> for the example above.
asm/gic.h provides the full definition. There looks to be exactly one case
(smp.c) where asm/vgic.h is included but asm/gic.h isn't. As there may be
peculiarities to this, Arm maintainers would need to take a look to decide
whether also including the other header that is an option.
>>> U8) Conversion between incompatible pointer types.
>>
>> Do we have any uses that aren't, by using casts, documenting that the
>> conversions are deliberate? Otherwise I would expect the compiler to
>> warn, and hence the build to fail due to -Werror. Then again I'm sure
>> we have ample uses of casts left which are actually bogus.
>
> Examples:
>
> xen/common/kernel.c:552.18-552.47:
> implicit cast converts from `const __typeof__(*(¶ms))*' (that is `const
> struct xen_platform_parameters*') to `void*' (ill-formed for the C99
> standard, ISO/IEC 9899:1999 Section 6.5.16.1: "Implicit conversion from a
> pointer to an incompatible pointer." [STD.pteincmp]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
> xen/include/xen/guest_access.h:65.23-65.24: expanded from macro
> `copy_to_guest_offset'
> xen/include/xen/guest_access.h:104.5-104.41: expanded from macro
> `copy_to_guest'
>
> xen/common/kernel.c:566.14-566.59:
> implicit cast converts from `const __typeof__(*(chgset))*' (that is `const
> char*') to `void*' (ill-formed for the C99 standard, ISO/IEC 9899:1999
> Section 6.5.16.1: "Implicit conversion from a pointer to an incompatible
> pointer." [STD.pteincmp]). Tool used is `/usr/bin/aarch64-linux-gnu-gcc-12'
> xen/include/xen/guest_access.h:65.23-65.24: expanded from macro
> `copy_to_guest_offset'
> xen/include/xen/guest_access.h:104.5-104.41: expanded from macro
> `copy_to_guest'
>
> xen/common/kernel.c:613.14-613.41:
> implicit cast converts from `const __typeof__(*(&fi))*' (that is `const
> struct xen_feature_info*') to `void*' (ill-formed for the C99 standard,
> ISO/IEC 9899:1999 Section 6.5.16.1: "Implicit conversion from a pointer to an
> incompatible pointer." [STD.pteincmp]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
> xen/include/xen/guest_access.h:123.23-123.24: expanded from macro
> `__copy_to_guest_offset'
> xen/include/xen/guest_access.h:152.5-152.43: expanded from macro
> `__copy_to_guest'
>
> xen/common/kernel.c:645.14-645.71:
> implicit cast converts from `const __typeof__(*(deny ? xen_deny() :
> saved_cmdline))*' (that is `const char*') to `void*' (ill-formed for the C99
> standard, ISO/IEC 9899:1999 Section 6.5.16.1: "Implicit conversion from a
> pointer to an incompatible pointer." [STD.pteincmp]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
> xen/include/xen/guest_access.h:65.23-65.24: expanded from macro
> `copy_to_guest_offset'
> xen/include/xen/guest_access.h:104.5-104.41: expanded from macro
> `copy_to_guest'
>
> xen/common/memory.c:1745.20-1745.53:
> implicit cast converts from `const __typeof__(*(&topology))*' (that is `const
> struct xen_vnuma_topology_info*') to `void*' (ill-formed for the C99
> standard, ISO/IEC 9899:1999 Section 6.5.16.1: "Implicit conversion from a
> pointer to an incompatible pointer." [STD.pteincmp]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
> xen/include/xen/guest_access.h:123.23-123.24: expanded from macro
> `__copy_to_guest_offset'
> xen/include/xen/guest_access.h:152.5-152.43: expanded from macro
> `__copy_to_guest'
>
> xen/common/memory.c:1808.14-1808.47:
> implicit cast converts from `const __typeof__(*(&topology))*' (that is `const
> struct xen_vnuma_topology_info*') to `void*' (ill-formed for the C99
> standard, ISO/IEC 9899:1999 Section 6.5.16.1: "Implicit conversion from a
> pointer to an incompatible pointer." [STD.pteincmp]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
> xen/include/xen/guest_access.h:123.23-123.24: expanded from macro
> `__copy_to_guest_offset'
> xen/include/xen/guest_access.h:152.5-152.43: expanded from macro
> `__copy_to_guest'
>
> xen/common/memory.c:1841.14-1841.47:
> implicit cast converts from `const __typeof__(*(&grdm.map))*' (that is `const
> struct xen_reserved_device_memory_map*') to `void*' (ill-formed for the C99
> standard, ISO/IEC 9899:1999 Section 6.5.16.1: "Implicit conversion from a
> pointer to an incompatible pointer." [STD.pteincmp]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
> xen/include/xen/guest_access.h:123.23-123.24: expanded from macro
> `__copy_to_guest_offset'
> xen/include/xen/guest_access.h:152.5-152.43: expanded from macro
> `__copy_to_guest'
These are all a common pattern. Line 65 in xen/guest_access.h is
(void)((hnd).p == _s); \
i.e. not an assignment in the first place. Nearby assignments don't
involve _s, yet that's pretty clearly what is being referred to by
const __typeof__(*(...))*. In pointer comparisons, differring
qualifiers are okay afaik (6.5.9), so I wonder whether the tool is
becoming confused here.
I don't know the acronyms used, but isn't STD.pteincmp referring to
comparisons rather that assignments?
> xen/common/efi/boot.c:335.16-335.56:
> implicit cast converts from `const CHAR16*' (that is `const unsigned short*')
> to `void*' (ill-formed for the C99 standard, ISO/IEC 9899:1999 Section
> 6.5.16.1: "Implicit conversion from a pointer to an incompatible pointer."
> [STD.pteincmp]). Tool used is `/usr/bin/aarch64-linux-gnu-gcc-12'
Same here - comparison of the result of wmemchr() (const CHAR16*)
with an expression derived from "data" (void*).
> xen/common/libfdt/fdt_overlay.c:733.69-733.87:
> implicit cast converts from `const char*' to `void*' (ill-formed for the C99
> standard, ISO/IEC 9899:1999 Section 6.5.16.1: "Implicit conversion from a
> pointer to an incompatible pointer." [STD.pteincmp]). Tool used is
> `/usr/bin/aarch64-linux-gnu-gcc-12'
Same here again, I think.
>>> U9) Conversion between function pointers and object pointers.
>
> Example:
> xen/arch/x86/apic.c:1159.16-1159.55: Loc #1 [culprit: c_style cast converts
> from `const void*' to `unsigned(*)(void)' (ill-formed for the C99 standard,
> ISO/IEC 9899:1999 Annex J.5.7: "A pointer to a function is converted to a
> pointer to an object or a pointer to an object is converted to a pointer to a
> function (6.5.4, 6.3.2.3)." [STD.funojptr]). Tool used is
> `/usr/bin/x86_64-linux-gnu-gcc-12']
>
>> Uses I'm readily aware of are deliberate. Plus isn't this shorthand
>> for going through uintptr_t intermediately only anyway?
>
> I don't understand the last sentence.
6.3.2.3 allows conversions between integer and pointer types. Hence
isn't intermediately going through uintptr_t merely a more verbose
way of casting between function and object pointers directly:
ptr = (void *)(uintptr_t)func;
vs
ptr = (void *)func;
?
>>> Here is a list of extensions that are documented in the GCC manual:
>>
>> I suppose that this list wasn't meant to be complete? The most
>> prominent example is probably asm().
>
> As far as I can tell the list was almost complete (I realize now
> that the use of the keyword __signed__ was omitted because
> investigation was not completed). But I am probably misunderstanding
> you.
Well, your list didn't include asm(), which is an extension we can't
do without. We also use __attribute__(()) extensively. And while
hunting for extensions which aren't documented in their own dedicated
sections, I think I had come across more. I didn't write these down,
and if at all possible I'd prefer to avoid repeating the exercise.
Jan
