Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

Hello Markus,

On Fri, 13 Oct 2023 15:28, Markus Armbruster <armbru@redhat.com> wrote:
>The commit message needs to explain why.

Certainly.

>This is wrong.  docs/devel/style.rst:
>
>    Include directives
>    ------------------
>
>    Order include directives as follows:
>
>    .. code-block:: c
>
>        #include "qemu/osdep.h"  /* Always first... */
>        #include <...>           /* then system headers... */
>        #include "..."           /* and finally QEMU headers. */
>
>Separate patch, please.

I know. spa headers use the `fallthrough` attribute and qemu/compiler.h 
defines it as a macro, so it breaks compilation. If the spa headers go 
after, we'd need to undef fallthrough before including them and 
re-define or re-include qemu/compiler.h after. What do you think would 
be best?

>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index 1109482a00..959982805d 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -1,215 +1,231 @@
>
>[...]
>
>>  #define QEMU_ALWAYS_INLINE
>>  #endif
>>  
>> -/**
>> - * In most cases, normal "fallthrough" comments are good enough for
>> - * switch-case statements, but sometimes the compiler has problems
>> - * with those. In that case you can use QEMU_FALLTHROUGH instead.
>> +/*
>> + * Add the pseudo keyword 'fallthrough' so case statement blocks
>
>Pseudo-keyword?  It's a macro.

C calls reserved words that you cannot redefine 'keywords'. Like 
'break', 'continue', 'return'. Hence it's a pseudo-keyword. It's also a 
macro. They are not mutually exclusive.

I did not write this, it was taken verbatim from the linux kernel 
source, see: /include/linux/compiler_attributes.h

>
>> + * must end with any of these keywords:
>> + *   break;
>> + *   fallthrough;
>> + *   continue;
>> + *   goto <label>;
>> + *   return [expression];
>
>These are statements, not keywords.

I'm pretty sure it refers to {break, fallthrough, continue, goto, 
return} by themselves.

>
>> + *
>> + *  gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
>
>Not sure we need to point to the docs here.  We have hundreds of
>__attribute__ uses in the tree.

Again, copied from /include/linux/compiler_attributes.h

>
>>   */
>> -#if __has_attribute(fallthrough)
>> -# define QEMU_FALLTHROUGH __attribute__((fallthrough))
>> +
>> +/*
>> + * glib_macros.h contains its own definition of fallthrough, so if we define
>> + * the pseudokeyword here it will expand when the glib header checks for the
>> + * attribute. glib headers must be #included after this header.
>> + */
>> +#ifdef fallthrough
>> +#undef fallthrough
>> +#endif
>
>Why do we need to roll our own macro then?

Glib uses a different name. The problem is when it checks for the 
compiler attribute, which expands into our macro.


--
Manos