Re: [PATCH] include: move typeof_strip_qual to compiler.h, use it in QAPI_LIST_LENGTH()

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

On Tue, 25 Jun 2024 14:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
>The typeof_strip_qual() is most useful for the atomic fetch-and-modify
>operations in atomic.h, but it can be used elsewhere as well.  For example,
>QAPI_LIST_LENGTH() assumes that the argument is not const, which is not a
>requirement.
>
>Move the macro to compiler.h and, while at it, move it under #ifndef
>__cplusplus to emphasize that it uses C-only constructs.  A C++ version
>of typeof_strip_qual() using type traits is possible[1], but beyond the
>scope of this patch because the little C++ code that is in QEMU does not
>use QAPI.
>
>The patch was tested by changing the declaration of strv_from_str_list()
>in qapi/qapi-type-helpers.c to:
>
>    char **strv_from_str_list(const strList *const list)
>
>This is valid C code, and it fails to compile without this change.
>
>[1] https://lore.kernel.org/qemu-devel/20240624205647.112034-1-flwu@google.com/
>
>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>---
> include/qapi/util.h     |  2 +-
> include/qemu/atomic.h   | 42 -------------------------------------
> include/qemu/compiler.h | 46 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 47 insertions(+), 43 deletions(-)
>
>diff --git a/include/qapi/util.h b/include/qapi/util.h
>index 20dfea8a545..b8254247b8d 100644
>--- a/include/qapi/util.h
>+++ b/include/qapi/util.h
>@@ -62,7 +62,7 @@ int parse_qapi_name(const char *name, bool complete);
> #define QAPI_LIST_LENGTH(list)                                      \
>     ({                                                              \
>         size_t _len = 0;                                            \
>-        typeof(list) _tail;                                         \
>+        typeof_strip_qual(list) _tail;                              \
>         for (_tail = list; _tail != NULL; _tail = _tail->next) {    \
>             _len++;                                                 \
>         }                                                           \
>diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
>index 99110abefb3..dc4118ddd9e 100644
>--- a/include/qemu/atomic.h
>+++ b/include/qemu/atomic.h
>@@ -20,48 +20,6 @@
> /* Compiler barrier */
> #define barrier()   ({ asm volatile("" ::: "memory"); (void)0; })
> 
>-/* The variable that receives the old value of an atomically-accessed
>- * variable must be non-qualified, because atomic builtins return values
>- * through a pointer-type argument as in __atomic_load(&var, &old, MODEL).
>- *
>- * This macro has to handle types smaller than int manually, because of
>- * implicit promotion.  int and larger types, as well as pointers, can be
>- * converted to a non-qualified type just by applying a binary operator.
>- */
>-#define typeof_strip_qual(expr)                                                    \
>-  typeof(                                                                          \
>-    __builtin_choose_expr(                                                         \
>-      __builtin_types_compatible_p(typeof(expr), bool) ||                          \
>-        __builtin_types_compatible_p(typeof(expr), const bool) ||                  \
>-        __builtin_types_compatible_p(typeof(expr), volatile bool) ||               \
>-        __builtin_types_compatible_p(typeof(expr), const volatile bool),           \
>-        (bool)1,                                                                   \
>-    __builtin_choose_expr(                                                         \
>-      __builtin_types_compatible_p(typeof(expr), signed char) ||                   \
>-        __builtin_types_compatible_p(typeof(expr), const signed char) ||           \
>-        __builtin_types_compatible_p(typeof(expr), volatile signed char) ||        \
>-        __builtin_types_compatible_p(typeof(expr), const volatile signed char),    \
>-        (signed char)1,                                                            \
>-    __builtin_choose_expr(                                                         \
>-      __builtin_types_compatible_p(typeof(expr), unsigned char) ||                 \
>-        __builtin_types_compatible_p(typeof(expr), const unsigned char) ||         \
>-        __builtin_types_compatible_p(typeof(expr), volatile unsigned char) ||      \
>-        __builtin_types_compatible_p(typeof(expr), const volatile unsigned char),  \
>-        (unsigned char)1,                                                          \
>-    __builtin_choose_expr(                                                         \
>-      __builtin_types_compatible_p(typeof(expr), signed short) ||                  \
>-        __builtin_types_compatible_p(typeof(expr), const signed short) ||          \
>-        __builtin_types_compatible_p(typeof(expr), volatile signed short) ||       \
>-        __builtin_types_compatible_p(typeof(expr), const volatile signed short),   \
>-        (signed short)1,                                                           \
>-    __builtin_choose_expr(                                                         \
>-      __builtin_types_compatible_p(typeof(expr), unsigned short) ||                \
>-        __builtin_types_compatible_p(typeof(expr), const unsigned short) ||        \
>-        __builtin_types_compatible_p(typeof(expr), volatile unsigned short) ||     \
>-        __builtin_types_compatible_p(typeof(expr), const volatile unsigned short), \
>-        (unsigned short)1,                                                         \
>-      (expr)+0))))))
>-
> #ifndef __ATOMIC_RELAXED
> #error "Expecting C11 atomic ops"
> #endif
>diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>index c797f0d4572..554c5ce7df7 100644
>--- a/include/qemu/compiler.h
>+++ b/include/qemu/compiler.h
>@@ -227,4 +227,50 @@
> #define SECOND_ARG(first, second, ...) second
> #define IS_EMPTY_(junk_maybecomma)     SECOND_ARG(junk_maybecomma 1, 0)
> 
>+#ifndef __cplusplus
>+/*
>+ * Useful in macros that need to declare temporary variables.  For example,
>+ * the variable that receives the old value of an atomically-accessed
>+ * variable must be non-qualified, because atomic builtins return values
>+ * through a pointer-type argument as in __atomic_load(&var, &old, MODEL).
>+ *
>+ * This macro has to handle types smaller than int manually, because of
>+ * implicit promotion.  int and larger types, as well as pointers, can be
>+ * converted to a non-qualified type just by applying a binary operator.
>+ */
>+#define typeof_strip_qual(expr)                                                    \
>+  typeof(                                                                          \
>+    __builtin_choose_expr(                                                         \
>+      __builtin_types_compatible_p(typeof(expr), bool) ||                          \
>+        __builtin_types_compatible_p(typeof(expr), const bool) ||                  \
>+        __builtin_types_compatible_p(typeof(expr), volatile bool) ||               \
>+        __builtin_types_compatible_p(typeof(expr), const volatile bool),           \
>+        (bool)1,                                                                   \
>+    __builtin_choose_expr(                                                         \
>+      __builtin_types_compatible_p(typeof(expr), signed char) ||                   \
>+        __builtin_types_compatible_p(typeof(expr), const signed char) ||           \
>+        __builtin_types_compatible_p(typeof(expr), volatile signed char) ||        \
>+        __builtin_types_compatible_p(typeof(expr), const volatile signed char),    \
>+        (signed char)1,                                                            \
>+    __builtin_choose_expr(                                                         \
>+      __builtin_types_compatible_p(typeof(expr), unsigned char) ||                 \
>+        __builtin_types_compatible_p(typeof(expr), const unsigned char) ||         \
>+        __builtin_types_compatible_p(typeof(expr), volatile unsigned char) ||      \
>+        __builtin_types_compatible_p(typeof(expr), const volatile unsigned char),  \
>+        (unsigned char)1,                                                          \
>+    __builtin_choose_expr(                                                         \
>+      __builtin_types_compatible_p(typeof(expr), signed short) ||                  \
>+        __builtin_types_compatible_p(typeof(expr), const signed short) ||          \
>+        __builtin_types_compatible_p(typeof(expr), volatile signed short) ||       \
>+        __builtin_types_compatible_p(typeof(expr), const volatile signed short),   \
>+        (signed short)1,                                                           \
>+    __builtin_choose_expr(                                                         \
>+      __builtin_types_compatible_p(typeof(expr), unsigned short) ||                \
>+        __builtin_types_compatible_p(typeof(expr), const unsigned short) ||        \
>+        __builtin_types_compatible_p(typeof(expr), volatile unsigned short) ||     \
>+        __builtin_types_compatible_p(typeof(expr), const volatile unsigned short), \
>+        (unsigned short)1,                                                         \
>+      (expr)+0))))))
>+#endif

Should we add an #else to provide a fallback for cplusplus until the 
alternative is merged?

-#endif
+#else /* __cpluplus */
+#define typeof_strip_qual typeof
+#endif /* __cplusplus */

>+
> #endif /* COMPILER_H */
>-- 
>2.45.2
>
>



With that comment addressed,

Tested-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>