| Message ID | 20240624205647.112034-1-flwu@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] include/qemu: Provide a C++ compatible version of typeof_strip_qual | expand |
Hi Felix, On 24/6/24 22:56, Felix Wu wrote: > From: Roman Kiryanov <rkir@google.com> > > to use the QEMU headers with a C++ compiler. > > Signed-off-by: Felix Wu <flwu@google.com> > Signed-off-by: Roman Kiryanov <rkir@google.com> > --- > include/qemu/atomic.h | 8 ++++++++ > include/qemu/atomic.hpp | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > create mode 100644 include/qemu/atomic.hpp > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index 99110abefb..aeaecc440a 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -20,6 +20,13 @@ > /* Compiler barrier */ > #define barrier() ({ asm volatile("" ::: "memory"); (void)0; }) > > +#ifdef __cplusplus > + > +#ifndef typeof_strip_qual > +#error Use the typeof_strip_qual(expr) definition from atomic.hpp on C++ builds. > +#endif > + > +#else /* __cpluplus */ > /* 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). > @@ -61,6 +68,7 @@ > __builtin_types_compatible_p(typeof(expr), const volatile unsigned short), \ > (unsigned short)1, \ > (expr)+0)))))) > +#endif /* __cpluplus */ > > #ifndef __ATOMIC_RELAXED > #error "Expecting C11 atomic ops" > diff --git a/include/qemu/atomic.hpp b/include/qemu/atomic.hpp > new file mode 100644 > index 0000000000..5844e3d427 > --- /dev/null > +++ b/include/qemu/atomic.hpp > @@ -0,0 +1,38 @@ > +/* > + * The C++ definition for typeof_strip_qual used in atomic.h. > + * > + * Copyright (C) 2024 Google, Inc. > + * > + * Author: Roman Kiryanov <rkir@google.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + * See docs/devel/atomics.rst for discussion about the guarantees each > + * atomic primitive is meant to provide. > + */ > + > +#ifndef QEMU_ATOMIC_HPP > +#define QEMU_ATOMIC_HPP > + > +#include <type_traits> > + > +/* Match the integer promotion behavior of typeof_strip_qual, see atomic.h */ > +template <class T> struct typeof_strip_qual_cpp { using result = decltype(+T(0)); }; > + > +template <> struct typeof_strip_qual_cpp<bool> { using result = bool; }; > +template <> struct typeof_strip_qual_cpp<signed char> { using result = signed char; }; > +template <> struct typeof_strip_qual_cpp<unsigned char> { using result = unsigned char; }; > +template <> struct typeof_strip_qual_cpp<signed short> { using result = signed short; }; > +template <> struct typeof_strip_qual_cpp<unsigned short> { using result = unsigned short; }; > + > +#define typeof_strip_qual(expr) \ > + typeof_strip_qual_cpp< \ > + std::remove_cv< \ > + std::remove_reference< \ > + decltype(expr) \ > + >::type \ > + >::type \ > + >::result > + > +#endif /* QEMU_ATOMIC_HPP */ As mentioned previously by Thomas, Daniel and Peter, mainstream QEMU doesn't use C++ and isn't being built-tested for it. I'm not against trying to keep the code C++ compatible, but I don't see the point of adding C++ files in the code base. In particular this patch seems contained well enough to be carried in forks were C++ _is_ used. Regards, Phil.
Hi Philippe, thank you for looking. On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > In particular this patch seems contained well enough > to be carried in forks were C++ _is_ used. Will you agree to take #ifdef __cplusplus and #error to the QEMU side in atomic.h and we will keep atomic.hpp on our side? The error message looks better when atomic.hpp is somewhere near. Regards, Roman.
Il mar 25 giu 2024, 04:32 Roman Kiryanov <rkir@google.com> ha scritto: > Hi Philippe, thank you for looking. > > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé > <philmd@linaro.org> wrote: > > In particular this patch seems contained well enough > > to be carried in forks were C++ _is_ used. > > Will you agree to take #ifdef __cplusplus and #error to the QEMU side > in atomic.h and > we will keep atomic.hpp on our side? The error message looks better > when atomic.hpp > is somewhere near. > I think we should also move typeof_strip_qual elsewhere; I will take a look. I think there are a couple headers that already have #ifdef __cplusplus, but I need to check (no source code around right now). But another good thing to do would be to avoid having atomic.h as a rebuild-the-world header, and any steps towards that would be very welcome. Paolo > Regards, > Roman. > >
On 25/06/2024 04.32, Roman Kiryanov wrote: > Hi Philippe, thank you for looking. > > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé > <philmd@linaro.org> wrote: >> In particular this patch seems contained well enough >> to be carried in forks were C++ _is_ used. > > Will you agree to take #ifdef __cplusplus and #error to the QEMU side > in atomic.h and > we will keep atomic.hpp on our side? Well, from an upstream QEMU perspective, it doesn't make sense to have an #error with a message that references a file that does not exist in the upstream repository, so I think that patch also rather belongs to your C++-enabled downstream repository. Thomas
On 25/6/24 08:05, Paolo Bonzini wrote: > > > Il mar 25 giu 2024, 04:32 Roman Kiryanov <rkir@google.com > <mailto:rkir@google.com>> ha scritto: > > Hi Philippe, thank you for looking. > > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > In particular this patch seems contained well enough > > to be carried in forks were C++ _is_ used. > > Will you agree to take #ifdef __cplusplus and #error to the QEMU side > in atomic.h and > we will keep atomic.hpp on our side? The error message looks better > when atomic.hpp > is somewhere near. > > > I think we should also move typeof_strip_qual elsewhere; I will take a > look. I think there are a couple headers that already have #ifdef > __cplusplus, but I need to check (no source code around right now). $ git grep -l __cplusplus ebpf/rss.bpf.skeleton.h include/hw/xtensa/xtensa-isa.h include/qemu/compiler.h include/qemu/osdep.h include/standard-headers/drm/drm_fourcc.h include/sysemu/os-posix.h include/sysemu/os-win32.h linux-headers/linux/stddef.h qga/vss-win32/requester.h > But another good thing to do would be to avoid having atomic.h as a > rebuild-the-world header, and any steps towards that would be very welcome. > > Paolo > > > Regards, > Roman. >
On Mon, Jun 24, 2024 at 08:56:47PM +0000, Felix Wu wrote: > From: Roman Kiryanov <rkir@google.com> > > to use the QEMU headers with a C++ compiler. > > Signed-off-by: Felix Wu <flwu@google.com> > Signed-off-by: Roman Kiryanov <rkir@google.com> > --- > include/qemu/atomic.h | 8 ++++++++ > include/qemu/atomic.hpp | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > create mode 100644 include/qemu/atomic.hpp > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index 99110abefb..aeaecc440a 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -20,6 +20,13 @@ > /* Compiler barrier */ > #define barrier() ({ asm volatile("" ::: "memory"); (void)0; }) > > +#ifdef __cplusplus > + > +#ifndef typeof_strip_qual > +#error Use the typeof_strip_qual(expr) definition from atomic.hpp on C++ builds. > +#endif > + > +#else /* __cpluplus */ > /* 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). > @@ -61,6 +68,7 @@ > __builtin_types_compatible_p(typeof(expr), const volatile unsigned short), \ > (unsigned short)1, \ > (expr)+0)))))) > +#endif /* __cpluplus */ > > #ifndef __ATOMIC_RELAXED > #error "Expecting C11 atomic ops" > diff --git a/include/qemu/atomic.hpp b/include/qemu/atomic.hpp > new file mode 100644 > index 0000000000..5844e3d427 > --- /dev/null > +++ b/include/qemu/atomic.hpp snip IMHO we don't want to see this code in QEMU - we are a C project, not a C++ project. With regards, Daniel
On Tue, 25 Jun 2024 at 07:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 25/6/24 08:05, Paolo Bonzini wrote: > > > > > > Il mar 25 giu 2024, 04:32 Roman Kiryanov <rkir@google.com > > <mailto:rkir@google.com>> ha scritto: > > > > Hi Philippe, thank you for looking. > > > > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé > > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > > In particular this patch seems contained well enough > > > to be carried in forks were C++ _is_ used. > > > > Will you agree to take #ifdef __cplusplus and #error to the QEMU side > > in atomic.h and > > we will keep atomic.hpp on our side? The error message looks better > > when atomic.hpp > > is somewhere near. > > > > > > I think we should also move typeof_strip_qual elsewhere; I will take a > > look. I think there are a couple headers that already have #ifdef > > __cplusplus, but I need to check (no source code around right now). > > $ git grep -l __cplusplus > ebpf/rss.bpf.skeleton.h > include/hw/xtensa/xtensa-isa.h > include/qemu/compiler.h > include/qemu/osdep.h > include/standard-headers/drm/drm_fourcc.h > include/sysemu/os-posix.h > include/sysemu/os-win32.h > linux-headers/linux/stddef.h > qga/vss-win32/requester.h We should delete all of those, they're dead code for us now. We dropped some of the extern-C-block handling in commit d76aa73fad1f6 but that didn't get all of them. thanks -- PMM
On Tue, 25 Jun 2024 at 10:27, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 25 Jun 2024 at 07:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > On 25/6/24 08:05, Paolo Bonzini wrote: > > > > > > > > > Il mar 25 giu 2024, 04:32 Roman Kiryanov <rkir@google.com > > > <mailto:rkir@google.com>> ha scritto: > > > > > > Hi Philippe, thank you for looking. > > > > > > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé > > > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > > > In particular this patch seems contained well enough > > > > to be carried in forks were C++ _is_ used. > > > > > > Will you agree to take #ifdef __cplusplus and #error to the QEMU side > > > in atomic.h and > > > we will keep atomic.hpp on our side? The error message looks better > > > when atomic.hpp > > > is somewhere near. > > > > > > > > > I think we should also move typeof_strip_qual elsewhere; I will take a > > > look. I think there are a couple headers that already have #ifdef > > > __cplusplus, but I need to check (no source code around right now). > > > > $ git grep -l __cplusplus > > ebpf/rss.bpf.skeleton.h > > include/hw/xtensa/xtensa-isa.h > > include/qemu/compiler.h > > include/qemu/osdep.h > > include/standard-headers/drm/drm_fourcc.h > > include/sysemu/os-posix.h > > include/sysemu/os-win32.h > > linux-headers/linux/stddef.h > > qga/vss-win32/requester.h > > We should delete all of those, they're dead code for us now. > We dropped some of the extern-C-block handling in > commit d76aa73fad1f6 but that didn't get all of them. I was wrong about this -- I had forgotten about the Windows Guest Agent code that has to be built with the Windows C++ compiler -- some of the cpp files in qga/vss-win32/ include osdep.h. So the files above break down into: * files imported from third-party projects, or generated by third-party tools: + ebpf/rss.bpf.skeleton.h + include/hw/xtensa/xtensa-isa.h + include/standard-headers/drm/drm_fourcc.h + linux-headers/linux/stddef.h * QEMU include files that we need to include directly or indirectly from the vss-win32 files: + include/qemu/compiler.h + include/qemu/osdep.h + include/sysemu/os-win32.h + include/sysemu/os-posix.h + qga/vss-win32/requester.h Maybe we could drop the cplusplus handling from os-posix.h, but I guess we're keeping it in parallel with os-win32.h. -- PMM
On Tue, Jun 25, 2024 at 11:16:16AM +0100, Peter Maydell wrote: > On Tue, 25 Jun 2024 at 10:27, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Tue, 25 Jun 2024 at 07:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > > > On 25/6/24 08:05, Paolo Bonzini wrote: > > > > > > > > > > > > Il mar 25 giu 2024, 04:32 Roman Kiryanov <rkir@google.com > > > > <mailto:rkir@google.com>> ha scritto: > > > > > > > > Hi Philippe, thank you for looking. > > > > > > > > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé > > > > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > > > > In particular this patch seems contained well enough > > > > > to be carried in forks were C++ _is_ used. > > > > > > > > Will you agree to take #ifdef __cplusplus and #error to the QEMU side > > > > in atomic.h and > > > > we will keep atomic.hpp on our side? The error message looks better > > > > when atomic.hpp > > > > is somewhere near. > > > > > > > > > > > > I think we should also move typeof_strip_qual elsewhere; I will take a > > > > look. I think there are a couple headers that already have #ifdef > > > > __cplusplus, but I need to check (no source code around right now). > > > > > > $ git grep -l __cplusplus > > > ebpf/rss.bpf.skeleton.h > > > include/hw/xtensa/xtensa-isa.h > > > include/qemu/compiler.h > > > include/qemu/osdep.h > > > include/standard-headers/drm/drm_fourcc.h > > > include/sysemu/os-posix.h > > > include/sysemu/os-win32.h > > > linux-headers/linux/stddef.h > > > qga/vss-win32/requester.h > > > > We should delete all of those, they're dead code for us now. > > We dropped some of the extern-C-block handling in > > commit d76aa73fad1f6 but that didn't get all of them. > > I was wrong about this -- I had forgotten about the Windows > Guest Agent code that has to be built with the Windows C++ > compiler -- some of the cpp files in qga/vss-win32/ include > osdep.h. So the files above break down into: > > * files imported from third-party projects, or generated > by third-party tools: > + ebpf/rss.bpf.skeleton.h > + include/hw/xtensa/xtensa-isa.h > + include/standard-headers/drm/drm_fourcc.h > + linux-headers/linux/stddef.h > > * QEMU include files that we need to include directly > or indirectly from the vss-win32 files: > + include/qemu/compiler.h > + include/qemu/osdep.h > + include/sysemu/os-win32.h > + include/sysemu/os-posix.h > + qga/vss-win32/requester.h > > Maybe we could drop the cplusplus handling from os-posix.h, > but I guess we're keeping it in parallel with os-win32.h. The vss-win32 code is specialized & self-contained code. Since it is inherantly Windows only code, it does not need any platform portability support which is what osdep.h would ordinarily assist with. As an example, if you remove osdep.h from the vss-win32 code, the following changes appear sufficient to solve the resulting compile issues. This could let us remove remaining cplusplus usage from the common QEMU headers: diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp index cc72e5ef1b..ed2d8097ee 100644 --- a/qga/vss-win32/provider.cpp +++ b/qga/vss-win32/provider.cpp @@ -10,7 +10,6 @@ * See the COPYING file in the top-level directory. */ -#include "qemu/osdep.h" #include "vss-common.h" #include "vss-debug.h" #ifdef HAVE_VSS_SDK diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp index 9884c65e70..e519e6cfd7 100644 --- a/qga/vss-win32/requester.cpp +++ b/qga/vss-win32/requester.cpp @@ -10,7 +10,6 @@ * See the COPYING file in the top-level directory. */ -#include "qemu/osdep.h" #include "vss-common.h" #include "vss-debug.h" #include "requester.h" diff --git a/qga/vss-win32/vss-common.h b/qga/vss-win32/vss-common.h index 0e67e7822c..5c6b21ce21 100644 --- a/qga/vss-win32/vss-common.h +++ b/qga/vss-win32/vss-common.h @@ -16,6 +16,9 @@ #define __MIDL_user_allocate_free_DEFINED__ #include <windows.h> #include <shlwapi.h> +#include <glib.h> +#include <assert.h> +#include "config-host.h" /* Reduce warnings to include vss.h */ diff --git a/qga/vss-win32/vss-debug.cpp b/qga/vss-win32/vss-debug.cpp index 820b1c6667..ec4c2b3093 100644 --- a/qga/vss-win32/vss-debug.cpp +++ b/qga/vss-win32/vss-debug.cpp @@ -10,7 +10,6 @@ * See the COPYING file in the top-level directory. */ -#include "qemu/osdep.h" #include "vss-debug.h" #include "vss-common.h" diff --git a/qga/vss-win32/vss-debug.h b/qga/vss-win32/vss-debug.h index 7800457392..77fd669698 100644 --- a/qga/vss-win32/vss-debug.h +++ b/qga/vss-win32/vss-debug.h @@ -10,8 +10,9 @@ * See the COPYING file in the top-level directory. */ -#include "qemu/osdep.h" #include <vss-handles.h> +#include <glib.h> +#include <stdio.h> #ifndef VSS_DEBUG_H #define VSS_DEBUG_H With regards, Daniel
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index 99110abefb..aeaecc440a 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -20,6 +20,13 @@ /* Compiler barrier */ #define barrier() ({ asm volatile("" ::: "memory"); (void)0; }) +#ifdef __cplusplus + +#ifndef typeof_strip_qual +#error Use the typeof_strip_qual(expr) definition from atomic.hpp on C++ builds. +#endif + +#else /* __cpluplus */ /* 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). @@ -61,6 +68,7 @@ __builtin_types_compatible_p(typeof(expr), const volatile unsigned short), \ (unsigned short)1, \ (expr)+0)))))) +#endif /* __cpluplus */ #ifndef __ATOMIC_RELAXED #error "Expecting C11 atomic ops" diff --git a/include/qemu/atomic.hpp b/include/qemu/atomic.hpp new file mode 100644 index 0000000000..5844e3d427 --- /dev/null +++ b/include/qemu/atomic.hpp @@ -0,0 +1,38 @@ +/* + * The C++ definition for typeof_strip_qual used in atomic.h. + * + * Copyright (C) 2024 Google, Inc. + * + * Author: Roman Kiryanov <rkir@google.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + * See docs/devel/atomics.rst for discussion about the guarantees each + * atomic primitive is meant to provide. + */ + +#ifndef QEMU_ATOMIC_HPP +#define QEMU_ATOMIC_HPP + +#include <type_traits> + +/* Match the integer promotion behavior of typeof_strip_qual, see atomic.h */ +template <class T> struct typeof_strip_qual_cpp { using result = decltype(+T(0)); }; + +template <> struct typeof_strip_qual_cpp<bool> { using result = bool; }; +template <> struct typeof_strip_qual_cpp<signed char> { using result = signed char; }; +template <> struct typeof_strip_qual_cpp<unsigned char> { using result = unsigned char; }; +template <> struct typeof_strip_qual_cpp<signed short> { using result = signed short; }; +template <> struct typeof_strip_qual_cpp<unsigned short> { using result = unsigned short; }; + +#define typeof_strip_qual(expr) \ + typeof_strip_qual_cpp< \ + std::remove_cv< \ + std::remove_reference< \ + decltype(expr) \ + >::type \ + >::type \ + >::result + +#endif /* QEMU_ATOMIC_HPP */