Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough
Hello Markus,
On Fri, 13 Oct 2023 15:28, Markus Armbruster 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 ;
+ * 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
Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough
The commit message needs to explain why.
Emmanouil Pitsidianakis writes:
> Signed-off-by: Emmanouil Pitsidianakis
> ---
> 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
> *
> * SPDX-License-Identifier: GPL-2.0-or-later
> */
>
> +#include
> +#include
> +#include
> +#include
> #include "qemu/osdep.h"
> #include "qemu/module.h"
> #include "audio.h"
> #include
> #include "qemu/error-report.h"
> #include "qapi/error.h"
> -#include
> -#include
> -#include
> -#include
>
> #include
> #include "trace.h"
>
> #define AUDIO_CAP "pipewire"
> #define RINGBUFFER_SIZE(1u << 22)
> #define RINGBUFFER_MASK(RINGBUFFER_SIZE - 1)
>
> #include "audio_int.h"
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.
[...]
> 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.
> + * must end with any of these keywords:
> + * break;
> + * fallthrough;
> + * continue;
> + * goto ;
> + * return [expression];
These are statements, not keywords.
> + *
> + * 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.
> */
> -#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?
> +
> +#if __has_attribute(__fallthrough__)
> +# define fallthrough__attribute__((__fallthrough__))
> #else
> -# define QEMU_FALLTHROUGH do {} while (0) /* fallthrough */
> +# define fallthroughdo {} while (0) /* fallthrough */
> #endif
>
> #ifdef CONFIG_CFI
> /*
[...]
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 475a1c62ff..8f790f0deb 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -1,171 +1,171 @@
> /*
> * OS includes and handling of OS dependencies
> *
> * This header exists to pull in some common system headers that
> * most code in QEMU will want, and to fix up some possible issues with
> * it (missing defines, Windows weirdness, and so on).
> *
> * To avoid getting into possible circular include dependencies, this
> * file should not include any other QEMU headers, with the exceptions
> * of config-host.h, config-target.h, qemu/compiler.h,
> * sysemu/os-posix.h, sysemu/os-win32.h, glib-compat.h and
> * qemu/typedefs.h, all of which are doing a similar job to this file
> * and are under similar constraints.
> *
> * This header also contains prototypes for functions defined in
> * os-*.c and util/oslib-*.c; those would probably be better split
> * out into separate header files.
> *
> * In an ideal world this header would contain only:
> * (1) things which everybody needs
> * (2) things without which code would work on most platforms but
> * fail to compile or misbehave on a minority of host OSes
> *
> * This work is licensed under the terms of the GNU GPL, version 2 or later.
> * See the COPYING file in the top-level directory.
> */
> #ifndef QEMU_OSDEP_H
> #define QEMU_OSDEP_H
>
> #if !defined _FORTIFY_SOURCE && defined __OPTIMIZE__ && __OPTIMIZE__ &&
> defined __linux__
> # define _FORTIFY_SOURCE 2
> #endif
>
> #include "config-host.h"
>
Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough
On Fri, 13 Oct 2023 at 11:16, Daniel P. Berrangé wrote: > This patch (and all the others in the series) have a ridiculously > large context either side of the change. It makes this horrible > to review as it requires wading through pages of pre-existing code > trying to spot the change. > > Please send patches with the default git context lines setting. Many thanks Daniel, I had not noticed at all. Somehow that option slipped through... Will reroll the patch series.
Re: [RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough
On Fri, Oct 13, 2023 at 10:47:05AM +0300, Emmanouil Pitsidianakis wrote:
> Signed-off-by: Emmanouil Pitsidianakis
> ---
> 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(-)
This patch (and all the others in the series) have a ridiculously
large context either side of the change. It makes this horrible
to review as it requires wading through pages of pre-existing code
trying to spot the change.
Please send patches with the default git context lines setting.
>
> 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
> *
> * SPDX-License-Identifier: GPL-2.0-or-later
> */
>
> +#include
> +#include
> +#include
> +#include
> #include "qemu/osdep.h"
> #include "qemu/module.h"
> #include "audio.h"
> #include
> #include "qemu/error-report.h"
> #include "qapi/error.h"
> -#include
> -#include
> -#include
> -#include
>
> #include
> #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)
> static int smmuv3_cmdq_consume(SMMUv3State *s)
> {
> SMMUState *bs = ARM_SMMU(s);
> SMMUCmdError cmd_error = SMMU_CERROR_NONE;
> SMMUQueue *q = >cmdq;
> SMMUCommandType type = 0;
>
> if (!smmuv3_cmdq_enabled(s)) {
> return 0;
> }
> /*
> * some commands depend on register values, typically CR0. In case those
> * register values change while handling the command, spec says it
> * is UNPREDICTABLE whether the command is interpreted under the new
> * or old value.
> */
>
> while (!smmuv3_q_empty(q)) {
> uint32_t pending = s->gerror ^ s->gerrorn;
> Cmd cmd;
>
> trace_smmuv3_cmdq_consume(Q_PROD(q), Q_CONS(q),
>Q_PROD_WRAP(q), Q_CONS_WRAP(q));
>
> if (FIELD_EX32(pending, GERROR, CMDQ_ERR)) {
> break;
> }
>
> if (queue_read(q, ) != MEMTX_OK) {
> cmd_error = SMMU_CERROR_ABT;
> break;
> }
>
> type = CMD_TYPE();
>
> trace_smmuv3_cmdq_opcode(smmu_cmd_string(type));
>
> qemu_mutex_lock(>mutex);
> switch (type) {
> case SMMU_CMD_SYNC:
> if (CMD_SYNC_CS() & CMD_SYNC_SIG_IRQ) {
> smmuv3_trigger_irq(s, SMMU_IRQ_CMD_SYNC, 0);
> }
> break;
> case SMMU_CMD_PREFETCH_CONFIG:
> case SMMU_CMD_PREFETCH_ADDR:
> break;
> case SMMU_CMD_CFGI_STE:
> {
> uint32_t sid = CMD_SID();
> IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid);
> SMMUDevice *sdev;
>
> if (CMD_SSEC()) {
> cmd_error = SMMU_CERROR_ILL;
> break;
> }
>
> if (!mr) {
> break;
> }
>
> trace_smmuv3_cmdq_cfgi_ste(sid);
> sdev = container_of(mr, SMMUDevice, iommu);
> smmuv3_flush_config(sdev);
>
> break;
> }
> case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL */
> {
> uint32_t sid = CMD_SID(), mask;
> uint8_t range = CMD_STE_RANGE();
> SMMUSIDRange sid_range;
>
> if (CMD_SSEC()) {
> cmd_error = SMMU_CERROR_ILL;
> break;
> }
>
> mask = (1ULL << (range + 1)) - 1;
> sid_range.start = sid & ~mask;
> sid_range.end = sid_range.start + mask;
>
> trace_smmuv3_cmdq_cfgi_ste_range(sid_range.start, sid_range.end);
> g_hash_table_foreach_remove(bs->configs, smmuv3_invalidate_ste,
> _range);
> break;
> }
> case SMMU_CMD_CFGI_CD:
> case SMMU_CMD_CFGI_CD_ALL:
> {
> uint32_t sid = CMD_SID();
> IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid);
> SMMUDevice *sdev;
>
> if (CMD_SSEC()) {
> cmd_error = SMMU_CERROR_ILL;
> break;
> }
>
>
[RFC PATCH 01/78] include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough
Signed-off-by: Emmanouil Pitsidianakis
---
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
*
* SPDX-License-Identifier: GPL-2.0-or-later
*/
+#include
+#include
+#include
+#include
#include "qemu/osdep.h"
#include "qemu/module.h"
#include "audio.h"
#include
#include "qemu/error-report.h"
#include "qapi/error.h"
-#include
-#include
-#include
-#include
#include
#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)
static int smmuv3_cmdq_consume(SMMUv3State *s)
{
SMMUState *bs = ARM_SMMU(s);
SMMUCmdError cmd_error = SMMU_CERROR_NONE;
SMMUQueue *q = >cmdq;
SMMUCommandType type = 0;
if (!smmuv3_cmdq_enabled(s)) {
return 0;
}
/*
* some commands depend on register values, typically CR0. In case those
* register values change while handling the command, spec says it
* is UNPREDICTABLE whether the command is interpreted under the new
* or old value.
*/
while (!smmuv3_q_empty(q)) {
uint32_t pending = s->gerror ^ s->gerrorn;
Cmd cmd;
trace_smmuv3_cmdq_consume(Q_PROD(q), Q_CONS(q),
Q_PROD_WRAP(q), Q_CONS_WRAP(q));
if (FIELD_EX32(pending, GERROR, CMDQ_ERR)) {
break;
}
if (queue_read(q, ) != MEMTX_OK) {
cmd_error = SMMU_CERROR_ABT;
break;
}
type = CMD_TYPE();
trace_smmuv3_cmdq_opcode(smmu_cmd_string(type));
qemu_mutex_lock(>mutex);
switch (type) {
case SMMU_CMD_SYNC:
if (CMD_SYNC_CS() & CMD_SYNC_SIG_IRQ) {
smmuv3_trigger_irq(s, SMMU_IRQ_CMD_SYNC, 0);
}
break;
case SMMU_CMD_PREFETCH_CONFIG:
case SMMU_CMD_PREFETCH_ADDR:
break;
case SMMU_CMD_CFGI_STE:
{
uint32_t sid = CMD_SID();
IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid);
SMMUDevice *sdev;
if (CMD_SSEC()) {
cmd_error = SMMU_CERROR_ILL;
break;
}
if (!mr) {
break;
}
trace_smmuv3_cmdq_cfgi_ste(sid);
sdev = container_of(mr, SMMUDevice, iommu);
smmuv3_flush_config(sdev);
break;
}
case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL */
{
uint32_t sid = CMD_SID(), mask;
uint8_t range = CMD_STE_RANGE();
SMMUSIDRange sid_range;
if (CMD_SSEC()) {
cmd_error = SMMU_CERROR_ILL;
break;
}
mask = (1ULL << (range + 1)) - 1;
sid_range.start = sid & ~mask;
sid_range.end = sid_range.start + mask;
trace_smmuv3_cmdq_cfgi_ste_range(sid_range.start, sid_range.end);
g_hash_table_foreach_remove(bs->configs, smmuv3_invalidate_ste,
_range);
break;
}
case SMMU_CMD_CFGI_CD:
case SMMU_CMD_CFGI_CD_ALL:
{
uint32_t sid = CMD_SID();
IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid);
SMMUDevice *sdev;
if (CMD_SSEC()) {
cmd_error = SMMU_CERROR_ILL;
break;
}
if (!mr) {
break;
}
trace_smmuv3_cmdq_cfgi_cd(sid);
sdev = container_of(mr, SMMUDevice, iommu);
smmuv3_flush_config(sdev);
break;
}
case SMMU_CMD_TLBI_NH_ASID:
{
uint16_t asid = CMD_ASID();
if (!STAGE1_SUPPORTED(s)) {
cmd_error = SMMU_CERROR_ILL;
break;
}
trace_smmuv3_cmdq_tlbi_nh_asid(asid);
smmu_inv_notifiers_all(>smmu_state);
smmu_iotlb_inv_asid(bs, asid);
break;
}
case SMMU_CMD_TLBI_NH_ALL:
