| Message ID | 20240625111848.709176-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | include: move typeof_strip_qual to compiler.h, use it in QAPI_LIST_LENGTH() | expand |
On 6/25/24 04:18, Paolo Bonzini 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(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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>
Hi Manos, thank you for looking. On Tue, Jun 25, 2024 at 12:17 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > 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 */ You probably want decltype for C++ and, unfortunately, you need somewhat more than just decltype to make this drop-in. I am +1 with this patch (without '#error something'), we will define typeof_strip_qual for C++ on our side. Regards, Roman.
On Tue, Jun 25, 2024 at 9:17 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > >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. > > Should we add an #else to provide a fallback for cplusplus until the > alternative is merged? As the commit message says, I don't think we need to include the C++ code in QEMU since it would be essentially dead. Paolo
On Thu, 27 Jun 2024 00:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >On Tue, Jun 25, 2024 at 9:17 PM Manos Pitsidianakis ><manos.pitsidianakis@linaro.org> wrote: >> >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. >> >> Should we add an #else to provide a fallback for cplusplus until the >> alternative is merged? > >As the commit message says, I don't think we need to include the C++ >code in QEMU since it would be essentially dead. The #ifndef __cplusplus part stood out for me, if it's not required for the qemu build then it's similarly unnecessary. Thinking out loud here. Anyway, ACK and tested-by and rob as in my previous mail. Thanks, Manos
On Thu, Jun 27, 2024 at 10:38 AM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > On Thu, 27 Jun 2024 00:32, Paolo Bonzini <pbonzini@redhat.com> wrote: > >On Tue, Jun 25, 2024 at 9:17 PM Manos Pitsidianakis > ><manos.pitsidianakis@linaro.org> wrote: > >> >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. > >> > >> Should we add an #else to provide a fallback for cplusplus until the > >> alternative is merged? > > > >As the commit message says, I don't think we need to include the C++ > >code in QEMU since it would be essentially dead. > > The #ifndef __cplusplus part stood out for me, if it's not required for > the qemu build then it's similarly unnecessary. Thinking out loud here. Yeah, the patch had actually two purposes, only one of which is explicit in the commit message because the other is more relevant to Google than to the upstream project: 1) Google wants some help making QEMU headers compilable with C++, which is generally not a goal of the project but not something we want to hinder. And if that brings to our attention things that could be improved in C as well, why not. 2) it is true that typeof_strip_qual() is useful in other places than qemu/atomic.h Since qemu/compiler.h already has #ifdef __cplusplus in various places, and could reasonably be included in the few C++ files that QEMU has, it makes sense to have a #ifndef __cplusplus in qemu/compiler.h (unlike in qemu/atomic.h). Personally I prefer to have a little more cognitive load in common headers such as qemu/osdep.h and qemu/compiler.h, than to have different styles (e.g. no qemu/osdep.h inclusion) in C++ sources, even though most likely I'll never touch those C++ sources. The #ifndef is not absolutely necessary---if we removed it, Google could work around that with "#ifdef __cplusplus / #undef typeof_strip_qual". It is mostly for documentation purposes to point out the C-only compiler builtins, and because if anyone ended up using typeof_strip_qual in C++ code the error message would be very very long. Paolo
On Thu, 27 Jun 2024 11:48, Paolo Bonzini <pbonzini@redhat.com> wrote: >because if anyone ended up using [..] C++ [..] the error message would >be very very long. I thought that comes with the territory
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 + #endif /* COMPILER_H */
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(-)