diff mbox series

[1/1] include/qemu: Provide a C++ compatible version of typeof_strip_qual

Message ID 20240624205647.112034-1-flwu@google.com (mailing list archive)
State New, archived
Headers show
Series [1/1] include/qemu: Provide a C++ compatible version of typeof_strip_qual | expand

Commit Message

Felix Wu June 24, 2024, 8:56 p.m. UTC
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

Comments

Philippe Mathieu-Daudé June 25, 2024, 2:26 a.m. UTC | #1
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.
Roman Kiryanov June 25, 2024, 2:32 a.m. UTC | #2
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.
Paolo Bonzini June 25, 2024, 6:05 a.m. UTC | #3
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.
>
>
Thomas Huth June 25, 2024, 6:07 a.m. UTC | #4
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
Philippe Mathieu-Daudé June 25, 2024, 6:19 a.m. UTC | #5
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.
>
Daniel P. Berrangé June 25, 2024, 7:52 a.m. UTC | #6
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
Peter Maydell June 25, 2024, 9:27 a.m. UTC | #7
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
Peter Maydell June 25, 2024, 10:16 a.m. UTC | #8
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
Daniel P. Berrangé June 25, 2024, 10:31 a.m. UTC | #9
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 mbox series

Patch

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 */