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(-)
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
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
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
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(-) >
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
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 > >
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(-) >
© 2016 - 2024 Red Hat, Inc.