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

[Eric Blake]

On Fri, Oct 13, 2023 at 10:56:28AM +0300, Emmanouil Pitsidianakis wrote:
> Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>

The subject line gives the 'what', but the commit body is
conspicuously lacking on the 'why'.

> ---
>  audio/pwaudio.c              |  8 ++++----
>  hw/arm/smmuv3.c              |  2 +-
>  include/qemu/compiler.h      | 30 +++++++++++++++++++++++-------
>  include/qemu/osdep.h         |  4 ++--
>  target/loongarch/cpu.c       |  4 ++--
>  target/loongarch/translate.c |  2 +-
>  tcg/optimize.c               |  8 ++++----
>  7 files changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/audio/pwaudio.c b/audio/pwaudio.c
> index 3ce5f6507b..bf26fadb06 100644
> --- a/audio/pwaudio.c
> +++ b/audio/pwaudio.c
> @@ -1,29 +1,29 @@
>  /*
>   * QEMU PipeWire audio driver
>   *
>   * Copyright (c) 2023 Red Hat Inc.
>   *
>   * Author: Dorinda Bassey       <dbassey@redhat.com>
>   *
>   * SPDX-License-Identifier: GPL-2.0-or-later
>   */
>  
> +#include <spa/param/audio/format-utils.h>
> +#include <spa/utils/ringbuffer.h>
> +#include <spa/utils/result.h>
> +#include <spa/param/props.h>
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "audio.h"
>  #include <errno.h>
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
> -#include <spa/param/audio/format-utils.h>
> -#include <spa/utils/ringbuffer.h>
> -#include <spa/utils/result.h>
> -#include <spa/param/props.h>

Rearranging #includes should be called out in the commit message, or
even better split into its own commit as it is distinct from renaming
the fallthrough macro.

>  
>  #include <pipewire/pipewire.h>
>  #include "trace.h"
>  
>  #define AUDIO_CAP "pipewire"
>  #define RINGBUFFER_SIZE    (1u << 22)
>  #define RINGBUFFER_MASK    (RINGBUFFER_SIZE - 1)
>  
>  #include "audio_int.h"
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 6f2b2bd45f..545d82ff04 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1166,210 +1166,210 @@ smmuv3_invalidate_ste(gpointer key, gpointer value, 
> gpointer user_data)

Wow, your git settings picked up a LOT of context - makes for a longer
email, but in this particular case, it IS nice to see the impact of
the fallthrough by seeing the full switch statment.

...
>          qemu_mutex_lock(&s->mutex);
>          switch (type) {
>          case SMMU_CMD_SYNC:
...
>          case SMMU_CMD_TLBI_NH_ALL:
>              if (!STAGE1_SUPPORTED(s)) {
>                  cmd_error = SMMU_CERROR_ILL;
>                  break;
>              }
> -            QEMU_FALLTHROUGH;
> +            fallthrough;
>          case SMMU_CMD_TLBI_NSNH_ALL:

Yes, that still looks legible to me.

> +++ b/include/qemu/compiler.h
> @@ -1,215 +1,231 @@
...
>  #if defined(__OPTIMIZE__)
>  #define QEMU_ALWAYS_INLINE  __attribute__((always_inline))
>  #else
>  #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
> + * must end with any of these keywords:
> + *   break;
> + *   fallthrough;
> + *   continue;
> + *   goto <label>;
> + *   return [expression];
> + *
> + *  gcc: 
> https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
>   */
> -#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
> +
> +#if __has_attribute(__fallthrough__)
> +# define fallthrough                    __attribute__((__fallthrough__))
>  #else
> -# define QEMU_FALLTHROUGH do {} while (0) /* fallthrough */
> +# define fallthrough                    do {} while (0)  /* fallthrough */
>  #endif
>

Looks okay.

> +++ b/include/qemu/osdep.h
> @@ -1,171 +1,171 @@
>  /*
>   * TARGET_WORDS_BIGENDIAN was replaced with TARGET_BIG_ENDIAN. Prevent it 
> from
>   * creeping back in.
>   */
>  #pragma GCC poison TARGET_WORDS_BIGENDIAN
>  
> -#include "qemu/compiler.h"
> -
>  /* Older versions of C++ don't get definitions of various macros from
>   * stdlib.h unless we define these macros before first inclusion of
>   * that system header.
>   */
...
>  /*
>   * This is somewhat like a system header; it must be outside any extern "C"
>   * block because it includes system headers itself, including glib.h,
>   * which will not compile if inside an extern "C" block.
>   */
>  #include "glib-compat.h"
>  
> +#include "qemu/compiler.h"
> +

The commit message should detail why we had to sink this include
later, and any audit you did to ensure that none of the intermediate
code is impacted by that change.

At this point, since the series is RFC, I'll leave merely:

Acked-by: Eric Blake <eblake@redhat.com>

but if others like the approach, I'll probably replace it with R-b in
the next version.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org