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

Peter Maydell posted 6 patches 3 weeks, 1 day ago
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210416135543.20382-1-peter.maydell@linaro.org
Maintainers: Stefan Weil <sw@weilnetz.de>, Peter Maydell <peter.maydell@linaro.org>, Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>
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(-)

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

Posted by Peter Maydell 3 weeks, 1 day ago
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


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

Posted by Paolo Bonzini 3 weeks, 1 day ago
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

Posted by Peter Maydell 3 weeks, 1 day ago
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? 0/6] extern "C" overhaul for C++ files

Posted by Paolo Bonzini 3 weeks, 1 day ago
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
>
>

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

Posted by Philippe Mathieu-Daudé 3 weeks, 1 day ago
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? 0/6] extern "C" overhaul for C++ files

Posted by no-reply@patchew.org 3 weeks, 1 day ago
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

Posted by Peter Maydell 4 days, 12 hours ago
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