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

[Manos Pitsidianakis]

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