[PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

[PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

[Peter Maydell]

Hi; this patchseries is:
 (1) a respin of Paolo's patches, with the review issue Dan
     noticed fixed (ie handle arm-a64.cc too)
 (2) a copy of my "osdep.h: Move system includes to top" patch
 (3) some new patches which try to more comprehensively address
     the extern "C" issue

I've marked this "for-6.0?", but more specifically:
 * I think patches 1 and 2 should go in if we do an rc4
   (and maybe we should do an rc4 given various things that
   have appeared that aren't individually rc4-worthy)
 * patches 3-6 are definitely 6.1 material

We have 2 C++ files in the tree which need to include QEMU
headers: disas/arm-a64.cc and disas/nanomips.cpp. These
include only osdep.h and dis-asm.h, so it is sufficient to
extern-C-ify those two files only.

I'm not wildly enthusiastic about this because it's pretty
invasive (and needs extending if we ever find we need to
include further headers from C++), but it seems to be what
C++ forces upon us...

Patches 1, 2 and 3 have been reviewed (I kept Dan's r-by on
patch 1 since the change to it is just fixing the thing he
noticed). Further review, and opinions on the 6.0-ness, whether
we should do an rc4, etc, appreciated.

thanks
-- PMM

Paolo Bonzini (2):
  osdep: include glib-compat.h before other QEMU headers
  osdep: protect qemu/osdep.h with extern "C"

Peter Maydell (4):
  include/qemu/osdep.h: Move system includes to top
  osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves
  include/qemu/bswap.h: Handle being included outside extern "C" block
  include/disas/dis-asm.h: Handle being included outside 'extern "C"'

 include/disas/dis-asm.h   | 12 ++++++++++--
 include/qemu/bswap.h      | 26 ++++++++++++++++++++++----
 include/qemu/compiler.h   |  6 ++++++
 include/qemu/osdep.h      | 34 +++++++++++++++++++++++++++-------
 include/sysemu/os-posix.h |  8 ++++++++
 include/sysemu/os-win32.h |  8 ++++++++
 disas/arm-a64.cc          |  2 --
 disas/nanomips.cpp        |  2 --
 8 files changed, 81 insertions(+), 17 deletions(-)

-- 
2.20.1

[PATCH for-6.0? 1/6] osdep: include glib-compat.h before other QEMU headers

[Peter Maydell]

From: Paolo Bonzini <pbonzini@redhat.com>

glib-compat.h is sort of like a system header, and it needs to include
system headers (glib.h) that may dislike being included under
'extern "C"'.  Move it right after all system headers and before
all other QEMU headers.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
[PMM: Added comment about why glib-compat.h is special]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/osdep.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ba15be9c569..ab84ecc7c1c 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -111,6 +111,13 @@ extern int daemon(int, int);
 #define WEXITSTATUS(x) (x)
 #endif
 
+/*
+ * This is somewhat like a system header; it must be outside any extern "C"
+ * block because it includes system headers itself, including glib.h,
+ * which will not compile if inside an extern "C" block.
+ */
+#include "glib-compat.h"
+
 #ifdef _WIN32
 #include "sysemu/os-win32.h"
 #endif
@@ -123,7 +130,6 @@ extern int daemon(int, int);
 #include <AvailabilityMacros.h>
 #endif
 
-#include "glib-compat.h"
 #include "qemu/typedefs.h"
 
 /*
-- 
2.20.1

[PATCH for-6.0? 2/6] osdep: protect qemu/osdep.h with extern "C"

[Peter Maydell]

From: Paolo Bonzini <pbonzini@redhat.com>

System headers may include templates if compiled with a C++ compiler,
which cause the compiler to complain if qemu/osdep.h is included
within a C++ source file's 'extern "C"' block.  Add
an 'extern "C"' block directly to qemu/osdep.h, so that
system headers can be kept out of it.

There is a stray declaration early in qemu/osdep.h, which needs
to be special cased.  Add a definition in qemu/compiler.h to
make it look nice.

config-host.h, CONFIG_TARGET, exec/poison.h and qemu/compiler.h
are included outside the 'extern "C"' block; that is not
an issue because they consist entirely of preprocessor directives.

This allows us to move the include of osdep.h in our two C++
source files outside the extern "C" block they were previously
using for it, which in turn means that they compile successfully
against newer versions of glib which insist that glib.h is
*not* inside an extern "C" block.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
[PMM: Moved disas/arm-a64.cc osdep.h include out of its extern "C" block;
 explained in commit message why we're doing this]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/compiler.h |  6 ++++++
 include/qemu/osdep.h    | 10 +++++++++-
 disas/arm-a64.cc        |  2 +-
 disas/nanomips.cpp      |  2 +-
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index cf28bb2bcd7..091c45248b0 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -11,6 +11,12 @@
 #define QEMU_STATIC_ANALYSIS 1
 #endif
 
+#ifdef __cplusplus
+#define QEMU_EXTERN_C extern "C"
+#else
+#define QEMU_EXTERN_C extern
+#endif
+
 #define QEMU_NORETURN __attribute__ ((__noreturn__))
 
 #define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ab84ecc7c1c..6d8562eaf40 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -57,7 +57,7 @@
 #define daemon qemu_fake_daemon_function
 #include <stdlib.h>
 #undef daemon
-extern int daemon(int, int);
+QEMU_EXTERN_C int daemon(int, int);
 #endif
 
 #ifdef _WIN32
@@ -118,6 +118,10 @@ extern int daemon(int, int);
  */
 #include "glib-compat.h"
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 #ifdef _WIN32
 #include "sysemu/os-win32.h"
 #endif
@@ -728,4 +732,8 @@ static inline int platform_does_not_support_system(const char *command)
 }
 #endif /* !HAVE_SYSTEM_FUNCTION */
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif
diff --git a/disas/arm-a64.cc b/disas/arm-a64.cc
index 9fa779e175e..27613d4b256 100644
--- a/disas/arm-a64.cc
+++ b/disas/arm-a64.cc
@@ -17,8 +17,8 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-extern "C" {
 #include "qemu/osdep.h"
+extern "C" {
 #include "disas/dis-asm.h"
 }
 
diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
index 2b096552719..8ddef897f0d 100644
--- a/disas/nanomips.cpp
+++ b/disas/nanomips.cpp
@@ -27,8 +27,8 @@
  *      Reference Manual", Revision 01.01, April 27, 2018
  */
 
-extern "C" {
 #include "qemu/osdep.h"
+extern "C" {
 #include "disas/dis-asm.h"
 }
 
-- 
2.20.1

[PATCH for-6.0? 3/6] include/qemu/osdep.h: Move system includes to top

[Peter Maydell]

Mostly osdep.h puts the system includes at the top of the file; but
there are a couple of exceptions where we include a system header
halfway through the file.  Move these up to the top with the rest
so that all the system headers we include are included before
we include os-win32.h or os-posix.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20210414184343.26235-1-peter.maydell@linaro.org
---
 include/qemu/osdep.h | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 6d8562eaf40..cb2a07e472e 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -104,6 +104,15 @@ QEMU_EXTERN_C int daemon(int, int);
 #include <setjmp.h>
 #include <signal.h>
 
+#ifdef CONFIG_IOVEC
+#include <sys/uio.h>
+#endif
+
+#if defined(__linux__) && defined(__sparc__)
+/* The SPARC definition of QEMU_VMALLOC_ALIGN needs SHMLBA */
+#include <sys/shm.h>
+#endif
+
 #ifndef _WIN32
 #include <sys/wait.h>
 #else
@@ -111,6 +120,10 @@ QEMU_EXTERN_C int daemon(int, int);
 #define WEXITSTATUS(x) (x)
 #endif
 
+#ifdef __APPLE__
+#include <AvailabilityMacros.h>
+#endif
+
 /*
  * This is somewhat like a system header; it must be outside any extern "C"
  * block because it includes system headers itself, including glib.h,
@@ -130,10 +143,6 @@ extern "C" {
 #include "sysemu/os-posix.h"
 #endif
 
-#ifdef __APPLE__
-#include <AvailabilityMacros.h>
-#endif
-
 #include "qemu/typedefs.h"
 
 /*
@@ -469,7 +478,6 @@ void qemu_anon_ram_free(void *ptr, size_t size);
    /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
 #  define QEMU_VMALLOC_ALIGN (256 * 4096)
 #elif defined(__linux__) && defined(__sparc__)
-#include <sys/shm.h>
 #  define QEMU_VMALLOC_ALIGN MAX(qemu_real_host_page_size, SHMLBA)
 #else
 #  define QEMU_VMALLOC_ALIGN qemu_real_host_page_size
@@ -549,8 +557,6 @@ struct iovec {
 
 ssize_t readv(int fd, const struct iovec *iov, int iov_cnt);
 ssize_t writev(int fd, const struct iovec *iov, int iov_cnt);
-#else
-#include <sys/uio.h>
 #endif
 
 #ifdef _WIN32
-- 
2.20.1

[PATCH for-6.0? 4/6] osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves

[Peter Maydell]

Both os-win32.h and os-posix.h include system header files. Instead
of having osdep.h include them inside its 'extern "C"' block, make
these headers handle that themselves, so that we don't include the
system headers inside 'extern "C"'.

This doesn't fix any current problems, but it's conceptually the
right way to handle system headers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/osdep.h      | 8 ++++----
 include/sysemu/os-posix.h | 8 ++++++++
 include/sysemu/os-win32.h | 8 ++++++++
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index cb2a07e472e..4c6f2390be4 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -131,10 +131,6 @@ QEMU_EXTERN_C int daemon(int, int);
  */
 #include "glib-compat.h"
 
-#ifdef __cplusplus
-extern "C" {
-#endif
-
 #ifdef _WIN32
 #include "sysemu/os-win32.h"
 #endif
@@ -143,6 +139,10 @@ extern "C" {
 #include "sysemu/os-posix.h"
 #endif
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 #include "qemu/typedefs.h"
 
 /*
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 629c8c648b7..2edf33658a4 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -38,6 +38,10 @@
 #include <sys/sysmacros.h>
 #endif
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 void os_set_line_buffering(void);
 void os_set_proc_name(const char *s);
 void os_setup_signal_handling(void);
@@ -92,4 +96,8 @@ static inline void qemu_funlockfile(FILE *f)
     funlockfile(f);
 }
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 5346d51e890..43f569b5c21 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -30,6 +30,10 @@
 #include <windows.h>
 #include <ws2tcpip.h>
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 #if defined(_WIN64)
 /* On w64, setjmp is implemented by _setjmp which needs a second parameter.
  * If this parameter is NULL, longjump does no stack unwinding.
@@ -194,4 +198,8 @@ ssize_t qemu_recv_wrap(int sockfd, void *buf, size_t len, int flags);
 ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, size_t len, int flags,
                            struct sockaddr *addr, socklen_t *addrlen);
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif
-- 
2.20.1

[PATCH for-6.0? 5/6] include/qemu/bswap.h: Handle being included outside extern "C" block

[Peter Maydell]

Make bswap.h handle being included outside an 'extern "C"' block:
all system headers are included first, then all declarations are
put inside an 'extern "C"' block.

This requires a little rearrangement as currently we have an ifdef
ladder that has some system includes and some local declarations
or definitions, and we need to separate those out.

We want to do this because dis-asm.h includes bswap.h, dis-asm.h
may need to be included from C++ files, and system headers should
not be included within 'extern "C"' blocks.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/bswap.h | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 4aaf992b5d7..2d3bb8bbedd 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -1,8 +1,6 @@
 #ifndef BSWAP_H
 #define BSWAP_H
 
-#include "fpu/softfloat-types.h"
-
 #ifdef CONFIG_MACHINE_BSWAP_H
 # include <sys/endian.h>
 # include <machine/bswap.h>
@@ -12,7 +10,18 @@
 # include <endian.h>
 #elif defined(CONFIG_BYTESWAP_H)
 # include <byteswap.h>
+#define BSWAP_FROM_BYTESWAP
+# else
+#define BSWAP_FROM_FALLBACKS
+#endif /* ! CONFIG_MACHINE_BSWAP_H */
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include "fpu/softfloat-types.h"
+
+#ifdef BSWAP_FROM_BYTESWAP
 static inline uint16_t bswap16(uint16_t x)
 {
     return bswap_16(x);
@@ -27,7 +36,9 @@ static inline uint64_t bswap64(uint64_t x)
 {
     return bswap_64(x);
 }
-# else
+#endif
+
+#ifdef BSWAP_FROM_FALLBACKS
 static inline uint16_t bswap16(uint16_t x)
 {
     return (((x & 0x00ff) << 8) |
@@ -53,7 +64,10 @@ static inline uint64_t bswap64(uint64_t x)
             ((x & 0x00ff000000000000ULL) >> 40) |
             ((x & 0xff00000000000000ULL) >> 56));
 }
-#endif /* ! CONFIG_MACHINE_BSWAP_H */
+#endif
+
+#undef BSWAP_FROM_BYTESWAP
+#undef BSWAP_FROM_FALLBACKS
 
 static inline void bswap16s(uint16_t *s)
 {
@@ -494,4 +508,8 @@ DO_STN_LDN_P(be)
 #undef le_bswaps
 #undef be_bswaps
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* BSWAP_H */
-- 
2.20.1

[PATCH for-6.0? 6/6] include/disas/dis-asm.h: Handle being included outside 'extern "C"'

[Peter Maydell]

Make dis-asm.h handle being included outside an 'extern "C"' block;
this allows us to remove the 'extern "C"' blocks that our two C++
files that include it are using.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/disas/dis-asm.h | 12 ++++++++++--
 disas/arm-a64.cc        |  2 --
 disas/nanomips.cpp      |  2 --
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/disas/dis-asm.h b/include/disas/dis-asm.h
index 13fa1edd411..4701445e806 100644
--- a/include/disas/dis-asm.h
+++ b/include/disas/dis-asm.h
@@ -9,6 +9,12 @@
 #ifndef DISAS_DIS_ASM_H
 #define DISAS_DIS_ASM_H
 
+#include "qemu/bswap.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 typedef void *PTR;
 typedef uint64_t bfd_vma;
 typedef int64_t bfd_signed_vma;
@@ -479,8 +485,6 @@ bool cap_disas_plugin(disassemble_info *info, uint64_t pc, size_t size);
 
 /* from libbfd */
 
-#include "qemu/bswap.h"
-
 static inline bfd_vma bfd_getl64(const bfd_byte *addr)
 {
     return ldq_le_p(addr);
@@ -508,4 +512,8 @@ static inline bfd_vma bfd_getb16(const bfd_byte *addr)
 
 typedef bool bfd_boolean;
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* DISAS_DIS_ASM_H */
diff --git a/disas/arm-a64.cc b/disas/arm-a64.cc
index 27613d4b256..a1402a2e071 100644
--- a/disas/arm-a64.cc
+++ b/disas/arm-a64.cc
@@ -18,9 +18,7 @@
  */
 
 #include "qemu/osdep.h"
-extern "C" {
 #include "disas/dis-asm.h"
-}
 
 #include "vixl/a64/disasm-a64.h"
 
diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
index 8ddef897f0d..9be8df75dd6 100644
--- a/disas/nanomips.cpp
+++ b/disas/nanomips.cpp
@@ -28,9 +28,7 @@
  */
 
 #include "qemu/osdep.h"
-extern "C" {
 #include "disas/dis-asm.h"
-}
 
 #include <cstring>
 #include <stdexcept>
-- 
2.20.1

Re: [PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210416135543.20382-1-peter.maydell@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210416135543.20382-1-peter.maydell@linaro.org
Subject: [PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210415104426.9860-1-valeriy.vdovin@virtuozzo.com -> patchew/20210415104426.9860-1-valeriy.vdovin@virtuozzo.com
 * [new tag]         patchew/20210416135543.20382-1-peter.maydell@linaro.org -> patchew/20210416135543.20382-1-peter.maydell@linaro.org
Switched to a new branch 'test'
30f73ae include/disas/dis-asm.h: Handle being included outside 'extern "C"'
cbe3886 include/qemu/bswap.h: Handle being included outside extern "C" block
ff73f93 osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves
ffb5bfc include/qemu/osdep.h: Move system includes to top
9c63702 osdep: protect qemu/osdep.h with extern "C"
bddc566 osdep: include glib-compat.h before other QEMU headers

=== OUTPUT BEGIN ===
1/6 Checking commit bddc5664e21c (osdep: include glib-compat.h before other QEMU headers)
2/6 Checking commit 9c6370297ba2 (osdep: protect qemu/osdep.h with extern "C")
WARNING: architecture specific defines should be avoided
#76: FILE: include/qemu/compiler.h:14:
+#ifdef __cplusplus

ERROR: storage class should be at the beginning of the declaration
#77: FILE: include/qemu/compiler.h:15:
+#define QEMU_EXTERN_C extern "C"

ERROR: storage class should be at the beginning of the declaration
#79: FILE: include/qemu/compiler.h:17:
+#define QEMU_EXTERN_C extern

WARNING: architecture specific defines should be avoided
#102: FILE: include/qemu/osdep.h:121:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#113: FILE: include/qemu/osdep.h:735:
+#ifdef __cplusplus

total: 2 errors, 3 warnings, 56 lines checked

Patch 2/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/6 Checking commit ffb5bfccb8a2 (include/qemu/osdep.h: Move system includes to top)
WARNING: architecture specific defines should be avoided
#34: FILE: include/qemu/osdep.h:111:
+#if defined(__linux__) && defined(__sparc__)

WARNING: architecture specific defines should be avoided
#46: FILE: include/qemu/osdep.h:123:
+#ifdef __APPLE__

total: 0 errors, 2 warnings, 50 lines checked

Patch 3/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/6 Checking commit ff73f933ce67 (osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves)
WARNING: architecture specific defines should be avoided
#42: FILE: include/qemu/osdep.h:142:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#57: FILE: include/sysemu/os-posix.h:41:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#68: FILE: include/sysemu/os-posix.h:99:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#81: FILE: include/sysemu/os-win32.h:33:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#92: FILE: include/sysemu/os-win32.h:201:
+#ifdef __cplusplus

total: 0 errors, 5 warnings, 56 lines checked

Patch 4/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/6 Checking commit cbe3886050e1 (include/qemu/bswap.h: Handle being included outside extern "C" block)
WARNING: architecture specific defines should be avoided
#47: FILE: include/qemu/bswap.h:18:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#84: FILE: include/qemu/bswap.h:511:
+#ifdef __cplusplus

total: 0 errors, 2 warnings, 55 lines checked

Patch 5/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/6 Checking commit 30f73aec5814 (include/disas/dis-asm.h: Handle being included outside 'extern "C"')
WARNING: architecture specific defines should be avoided
#57: FILE: include/disas/dis-asm.h:14:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#77: FILE: include/disas/dis-asm.h:515:
+#ifdef __cplusplus

total: 0 errors, 2 warnings, 46 lines checked

Patch 6/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210416135543.20382-1-peter.maydell@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

[Philippe Mathieu-Daudé]

Cc'ing MediaTek future contributors who expressed interest
in reviewing this.

On 4/16/21 3:55 PM, Peter Maydell wrote:
> Hi; this patchseries is:
>  (1) a respin of Paolo's patches, with the review issue Dan
>      noticed fixed (ie handle arm-a64.cc too)
>  (2) a copy of my "osdep.h: Move system includes to top" patch
>  (3) some new patches which try to more comprehensively address
>      the extern "C" issue
> 
> I've marked this "for-6.0?", but more specifically:
>  * I think patches 1 and 2 should go in if we do an rc4
>    (and maybe we should do an rc4 given various things that
>    have appeared that aren't individually rc4-worthy)
>  * patches 3-6 are definitely 6.1 material
> 
> We have 2 C++ files in the tree which need to include QEMU
> headers: disas/arm-a64.cc and disas/nanomips.cpp. These
> include only osdep.h and dis-asm.h, so it is sufficient to
> extern-C-ify those two files only.
> 
> I'm not wildly enthusiastic about this because it's pretty
> invasive (and needs extending if we ever find we need to
> include further headers from C++), but it seems to be what
> C++ forces upon us...
> 
> Patches 1, 2 and 3 have been reviewed (I kept Dan's r-by on
> patch 1 since the change to it is just fixing the thing he
> noticed). Further review, and opinions on the 6.0-ness, whether
> we should do an rc4, etc, appreciated.
> 
> thanks
> -- PMM
> 
> Paolo Bonzini (2):
>   osdep: include glib-compat.h before other QEMU headers
>   osdep: protect qemu/osdep.h with extern "C"
> 
> Peter Maydell (4):
>   include/qemu/osdep.h: Move system includes to top
>   osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves
>   include/qemu/bswap.h: Handle being included outside extern "C" block
>   include/disas/dis-asm.h: Handle being included outside 'extern "C"'
> 
>  include/disas/dis-asm.h   | 12 ++++++++++--
>  include/qemu/bswap.h      | 26 ++++++++++++++++++++++----
>  include/qemu/compiler.h   |  6 ++++++
>  include/qemu/osdep.h      | 34 +++++++++++++++++++++++++++-------
>  include/sysemu/os-posix.h |  8 ++++++++
>  include/sysemu/os-win32.h |  8 ++++++++
>  disas/arm-a64.cc          |  2 --
>  disas/nanomips.cpp        |  2 --
>  8 files changed, 81 insertions(+), 17 deletions(-)
> 

Re: [PATCH for-6.0? 4/6] osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves

[Paolo Bonzini]

On 16/04/21 15:55, Peter Maydell wrote:
>   #ifdef _WIN32
>   #include "sysemu/os-win32.h"
>   #endif
> @@ -143,6 +139,10 @@ extern "C" {
>   #include "sysemu/os-posix.h"
>   #endif
>   
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
>   #include "qemu/typedefs.h"
>   
>   /*
> diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
> index 629c8c648b7..2edf33658a4 100644
> --- a/include/sysemu/os-posix.h
> +++ b/include/sysemu/os-posix.h
> @@ -38,6 +38,10 @@
>   #include <sys/sysmacros.h>
>   #endif
>   
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +

include/sysemu/ is also the wrong directory to have these headers, which 
probably should be split into a qemu/osdep-{win32,posix}.h part and an 
actual sysemu/os-{win32,posix}.h part.  But this is good enough for now.

Paolo

Re: [PATCH for-6.0? 4/6] osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves

[Peter Maydell]

On Fri, 16 Apr 2021 at 17:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 16/04/21 15:55, Peter Maydell wrote:
> >   #ifdef _WIN32
> >   #include "sysemu/os-win32.h"
> >   #endif
> > @@ -143,6 +139,10 @@ extern "C" {
> >   #include "sysemu/os-posix.h"
> >   #endif
> >
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> >   #include "qemu/typedefs.h"
> >
> >   /*
> > diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
> > index 629c8c648b7..2edf33658a4 100644
> > --- a/include/sysemu/os-posix.h
> > +++ b/include/sysemu/os-posix.h
> > @@ -38,6 +38,10 @@
> >   #include <sys/sysmacros.h>
> >   #endif
> >
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
>
> include/sysemu/ is also the wrong directory to have these headers, which
> probably should be split into a qemu/osdep-{win32,posix}.h part and an
> actual sysemu/os-{win32,posix}.h part.  But this is good enough for now.

Yeah, I thought as I was writing this that it was odd that these are
in sysemu/ but osdep itself is in qemu/...

thanks
-- PMM

Re: [PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

[Paolo Bonzini]

On 16/04/21 15:55, Peter Maydell wrote:
> Hi; this patchseries is:
>   (1) a respin of Paolo's patches, with the review issue Dan
>       noticed fixed (ie handle arm-a64.cc too)
>   (2) a copy of my "osdep.h: Move system includes to top" patch
>   (3) some new patches which try to more comprehensively address
>       the extern "C" issue
> 
> I've marked this "for-6.0?", but more specifically:
>   * I think patches 1 and 2 should go in if we do an rc4
>     (and maybe we should do an rc4 given various things that
>     have appeared that aren't individually rc4-worthy)
>   * patches 3-6 are definitely 6.1 material
> 
> We have 2 C++ files in the tree which need to include QEMU
> headers: disas/arm-a64.cc and disas/nanomips.cpp. These
> include only osdep.h and dis-asm.h, so it is sufficient to
> extern-C-ify those two files only.
> 
> I'm not wildly enthusiastic about this because it's pretty
> invasive (and needs extending if we ever find we need to
> include further headers from C++), but it seems to be what
> C++ forces upon us...
> 
> Patches 1, 2 and 3 have been reviewed (I kept Dan's r-by on
> patch 1 since the change to it is just fixing the thing he
> noticed). Further review, and opinions on the 6.0-ness, whether
> we should do an rc4, etc, appreciated.

I think at least 1-3 are 6.0 material because build on a supported 
distro (Fedora 34, due for release on April 27) is broken right now.

For 4-6 I'm a bit less sure since there's more to cleanup in 
include/sysemu/os-*.h.  Anyway,

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> thanks
> -- PMM
> 
> Paolo Bonzini (2):
>    osdep: include glib-compat.h before other QEMU headers
>    osdep: protect qemu/osdep.h with extern "C"
> 
> Peter Maydell (4):
>    include/qemu/osdep.h: Move system includes to top
>    osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves
>    include/qemu/bswap.h: Handle being included outside extern "C" block
>    include/disas/dis-asm.h: Handle being included outside 'extern "C"'
> 
>   include/disas/dis-asm.h   | 12 ++++++++++--
>   include/qemu/bswap.h      | 26 ++++++++++++++++++++++----
>   include/qemu/compiler.h   |  6 ++++++
>   include/qemu/osdep.h      | 34 +++++++++++++++++++++++++++-------
>   include/sysemu/os-posix.h |  8 ++++++++
>   include/sysemu/os-win32.h |  8 ++++++++
>   disas/arm-a64.cc          |  2 --
>   disas/nanomips.cpp        |  2 --
>   8 files changed, 81 insertions(+), 17 deletions(-)
> 

Re: [PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

[Peter Maydell]

On Fri, 16 Apr 2021 at 17:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 16/04/21 15:55, Peter Maydell wrote:
> > Hi; this patchseries is:
> >   (1) a respin of Paolo's patches, with the review issue Dan
> >       noticed fixed (ie handle arm-a64.cc too)
> >   (2) a copy of my "osdep.h: Move system includes to top" patch
> >   (3) some new patches which try to more comprehensively address
> >       the extern "C" issue
> >
> > I've marked this "for-6.0?", but more specifically:
> >   * I think patches 1 and 2 should go in if we do an rc4
> >     (and maybe we should do an rc4 given various things that
> >     have appeared that aren't individually rc4-worthy)
> >   * patches 3-6 are definitely 6.1 material

> > Patches 1, 2 and 3 have been reviewed (I kept Dan's r-by on
> > patch 1 since the change to it is just fixing the thing he
> > noticed). Further review, and opinions on the 6.0-ness, whether
> > we should do an rc4, etc, appreciated.
>
> I think at least 1-3 are 6.0 material because build on a supported
> distro (Fedora 34, due for release on April 27) is broken right now.

We don't support not-yet-released distros, because there's no way
to do that: they might always spring new surprises on us that we
don't have time to react to. But I agree that the weight of stuff
we've built up justifies an rc4.

Is patch 3 also required? I thought 1 and 2 would suffice, but
I don't have a system which has the newer glib.

-- PMM

Re: [PATCH for-6.0? 1/6] osdep: include glib-compat.h before other QEMU headers

[Richard Henderson]

On 4/16/21 6:55 AM, Peter Maydell wrote:
> From: Paolo Bonzini<pbonzini@redhat.com>
> 
> glib-compat.h is sort of like a system header, and it needs to include
> system headers (glib.h) that may dislike being included under
> 'extern "C"'.  Move it right after all system headers and before
> all other QEMU headers.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> [PMM: Added comment about why glib-compat.h is special]
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/qemu/osdep.h | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-6.0? 2/6] osdep: protect qemu/osdep.h with extern "C"

[Richard Henderson]

On 4/16/21 6:55 AM, Peter Maydell wrote:
> From: Paolo Bonzini<pbonzini@redhat.com>
> 
> System headers may include templates if compiled with a C++ compiler,
> which cause the compiler to complain if qemu/osdep.h is included
> within a C++ source file's 'extern "C"' block.  Add
> an 'extern "C"' block directly to qemu/osdep.h, so that
> system headers can be kept out of it.
> 
> There is a stray declaration early in qemu/osdep.h, which needs
> to be special cased.  Add a definition in qemu/compiler.h to
> make it look nice.
> 
> config-host.h, CONFIG_TARGET, exec/poison.h and qemu/compiler.h
> are included outside the 'extern "C"' block; that is not
> an issue because they consist entirely of preprocessor directives.
> 
> This allows us to move the include of osdep.h in our two C++
> source files outside the extern "C" block they were previously
> using for it, which in turn means that they compile successfully
> against newer versions of glib which insist that glib.h is
> *not*  inside an extern "C" block.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
> [PMM: Moved disas/arm-a64.cc osdep.h include out of its extern "C" block;
>   explained in commit message why we're doing this]
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/qemu/compiler.h |  6 ++++++
>   include/qemu/osdep.h    | 10 +++++++++-
>   disas/arm-a64.cc        |  2 +-
>   disas/nanomips.cpp      |  2 +-
>   4 files changed, 17 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-6.0? 3/6] include/qemu/osdep.h: Move system includes to top

[Richard Henderson]

On 4/16/21 6:55 AM, Peter Maydell wrote:
> Mostly osdep.h puts the system includes at the top of the file; but
> there are a couple of exceptions where we include a system header
> halfway through the file.  Move these up to the top with the rest
> so that all the system headers we include are included before
> we include os-win32.h or os-posix.h.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
> Message-id:20210414184343.26235-1-peter.maydell@linaro.org
> ---
>   include/qemu/osdep.h | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-6.0? 4/6] osdep: Make os-win32.h and os-posix.h handle 'extern "C"' themselves

[Richard Henderson]

On 4/16/21 6:55 AM, Peter Maydell wrote:
> Both os-win32.h and os-posix.h include system header files. Instead
> of having osdep.h include them inside its 'extern "C"' block, make
> these headers handle that themselves, so that we don't include the
> system headers inside 'extern "C"'.
> 
> This doesn't fix any current problems, but it's conceptually the
> right way to handle system headers.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/qemu/osdep.h      | 8 ++++----
>   include/sysemu/os-posix.h | 8 ++++++++
>   include/sysemu/os-win32.h | 8 ++++++++
>   3 files changed, 20 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-6.0? 5/6] include/qemu/bswap.h: Handle being included outside extern "C" block

[Richard Henderson]

On 4/16/21 6:55 AM, Peter Maydell wrote:
> Make bswap.h handle being included outside an 'extern "C"' block:
> all system headers are included first, then all declarations are
> put inside an 'extern "C"' block.
> 
> This requires a little rearrangement as currently we have an ifdef
> ladder that has some system includes and some local declarations
> or definitions, and we need to separate those out.
> 
> We want to do this because dis-asm.h includes bswap.h, dis-asm.h
> may need to be included from C++ files, and system headers should
> not be included within 'extern "C"' blocks.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/qemu/bswap.h | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-6.0? 6/6] include/disas/dis-asm.h: Handle being included outside 'extern "C"'

[Richard Henderson]

On 4/16/21 6:55 AM, Peter Maydell wrote:
> Make dis-asm.h handle being included outside an 'extern "C"' block;
> this allows us to remove the 'extern "C"' blocks that our two C++
> files that include it are using.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/disas/dis-asm.h | 12 ++++++++++--
>   disas/arm-a64.cc        |  2 --
>   disas/nanomips.cpp      |  2 --
>   3 files changed, 10 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

Il ven 16 apr 2021, 19:08 Peter Maydell <peter.maydell@linaro.org> ha
scritto:

> > I think at least 1-3 are 6.0 material because build on a supported
> > distro (Fedora 34, due for release on April 27) is broken right now.
>
> We don't support not-yet-released distros, because there's no way
> to do that: they might always spring new surprises on us that we
> don't have time to react to. But I agree that the weight of stuff
> we've built up justifies an rc4.
>

Also we can expect that other distros will have the same issue over the
next 4 months. (In fact rolling release distros like Debian testing and
Arch might be already broken for what we know).

Is patch 3 also required? I thought 1 and 2 would suffice, but
> I don't have a system which has the newer glib.
>

Yes, they do.

Paolo


> -- PMM
>
>

[-- Attachment #2: Type: text/html, Size: 1679 bytes --]

Re: [PATCH for-6.0? 0/6] extern "C" overhaul for C++ files

[Peter Maydell]

On Fri, 16 Apr 2021 at 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Hi; this patchseries is:
>  (1) a respin of Paolo's patches, with the review issue Dan
>      noticed fixed (ie handle arm-a64.cc too)
>  (2) a copy of my "osdep.h: Move system includes to top" patch
>  (3) some new patches which try to more comprehensively address
>      the extern "C" issue
>
> I've marked this "for-6.0?", but more specifically:
>  * I think patches 1 and 2 should go in if we do an rc4
>    (and maybe we should do an rc4 given various things that
>    have appeared that aren't individually rc4-worthy)
>  * patches 3-6 are definitely 6.1 material
>
> We have 2 C++ files in the tree which need to include QEMU
> headers: disas/arm-a64.cc and disas/nanomips.cpp. These
> include only osdep.h and dis-asm.h, so it is sufficient to
> extern-C-ify those two files only.
>
> I'm not wildly enthusiastic about this because it's pretty
> invasive (and needs extending if we ever find we need to
> include further headers from C++), but it seems to be what
> C++ forces upon us...
>
> Patches 1, 2 and 3 have been reviewed (I kept Dan's r-by on
> patch 1 since the change to it is just fixing the thing he
> noticed). Further review, and opinions on the 6.0-ness, whether
> we should do an rc4, etc, appreciated.

1-3 went in for 6.0; I've now picked up 4-6 to go in via
target-arm.next.

thanks
-- PMM