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

2024-06-25 Thread Daniel P . Berrangé 
On Tue, Jun 25, 2024 at 11:16:16AM +0100, Peter Maydell wrote:
> On Tue, 25 Jun 2024 at 10:27, Peter Maydell  wrote:
> >
> > On Tue, 25 Jun 2024 at 07:19, Philippe Mathieu-Daudé  
> > wrote:
> > >
> > > On 25/6/24 08:05, Paolo Bonzini wrote:
> > > >
> > > >
> > > > Il mar 25 giu 2024, 04:32 Roman Kiryanov  > > > > ha scritto:
> > > >
> > > > Hi Philippe, thank you for looking.
> > > >
> > > > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé
> > > > mailto:phi...@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 
 #include 
+#include 
+#include 
+#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 
+#include 
+#include 
 
 #ifndef VSS_DEBUG_H
 #define VSS_DEBUG_H


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

2024-06-25 Thread Peter Maydell 
On Tue, 25 Jun 2024 at 10:27, Peter Maydell  wrote:
>
> On Tue, 25 Jun 2024 at 07:19, Philippe Mathieu-Daudé  
> wrote:
> >
> > On 25/6/24 08:05, Paolo Bonzini wrote:
> > >
> > >
> > > Il mar 25 giu 2024, 04:32 Roman Kiryanov  > > > ha scritto:
> > >
> > > Hi Philippe, thank you for looking.
> > >
> > > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé
> > > mailto:phi...@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

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

2024-06-25 Thread Peter Maydell 
On Tue, 25 Jun 2024 at 07:19, Philippe Mathieu-Daudé  wrote:
>
> On 25/6/24 08:05, Paolo Bonzini wrote:
> >
> >
> > Il mar 25 giu 2024, 04:32 Roman Kiryanov  > > ha scritto:
> >
> > Hi Philippe, thank you for looking.
> >
> > On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé
> > mailto:phi...@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

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

2024-06-25 Thread Daniel P . Berrangé 
On Mon, Jun 24, 2024 at 08:56:47PM +, Felix Wu wrote:
> From: Roman Kiryanov 
> 
> to use the QEMU headers with a C++ compiler.
> 
> Signed-off-by: Felix Wu 
> Signed-off-by: Roman Kiryanov 
> ---
>  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(, , 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 00..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
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

2024-06-25 Thread Philippe Mathieu-Daudé 

On 25/6/24 08:05, Paolo Bonzini wrote:



Il mar 25 giu 2024, 04:32 Roman Kiryanov > ha scritto:


Hi Philippe, thank you for looking.

On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé
mailto:phi...@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.

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

2024-06-25 Thread Thomas Huth 

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é
 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

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

2024-06-25 Thread Paolo Bonzini 
Il mar 25 giu 2024, 04:32 Roman Kiryanov  ha scritto:

> Hi Philippe, thank you for looking.
>
> On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé
>  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.
>
>

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

2024-06-24 Thread Roman Kiryanov 
Hi Philippe, thank you for looking.

On Mon, Jun 24, 2024 at 7:27 PM Philippe Mathieu-Daudé
 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.

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

2024-06-24 Thread Philippe Mathieu-Daudé 

Hi Felix,

On 24/6/24 22:56, Felix Wu wrote:

From: Roman Kiryanov 

to use the QEMU headers with a C++ compiler.

Signed-off-by: Felix Wu 
Signed-off-by: Roman Kiryanov 
---
  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(, , 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 00..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 
+ *
+ * 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 
+
+/* Match the integer promotion behavior of typeof_strip_qual, see atomic.h */
+template  struct typeof_strip_qual_cpp { using result = 
decltype(+T(0)); };
+
+template <> struct typeof_strip_qual_cpp { using result = bool; };
+template <> struct typeof_strip_qual_cpp { using result = signed 
char; };
+template <> struct typeof_strip_qual_cpp { using result = 
unsigned char; };
+template <> struct typeof_strip_qual_cpp { using result = signed 
short; };
+template <> struct typeof_strip_qual_cpp { 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.

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

2024-06-24 Thread Felix Wu 
From: Roman Kiryanov 

to use the QEMU headers with a C++ compiler.

Signed-off-by: Felix Wu 
Signed-off-by: Roman Kiryanov 
---
 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(, , 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 00..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 
+ *
+ * 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 
+
+/* Match the integer promotion behavior of typeof_strip_qual, see atomic.h */
+template  struct typeof_strip_qual_cpp { using result = 
decltype(+T(0)); };
+
+template <> struct typeof_strip_qual_cpp { using result = bool; };
+template <> struct typeof_strip_qual_cpp { using result = signed 
char; };
+template <> struct typeof_strip_qual_cpp { using result = 
unsigned char; };
+template <> struct typeof_strip_qual_cpp { using result = signed 
short; };
+template <> struct typeof_strip_qual_cpp { 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 */
-- 
2.45.2.741.gdbec12cfda-goog