1. Patchew
  2. QEMU
  3. View series

[PATCH] disas: Fix build with glib2.0 >=2.67.3

Christian Ehrhardt posted 1 patch 4 years, 2 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210223145646.4129643-1-christian.ehrhardt@canonical.com
disas/arm-a64.cc   | 2 +-
disas/nanomips.cpp | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
[PATCH] disas: Fix build with glib2.0 >=2.67.3
Posted by Christian Ehrhardt 4 years, 2 months ago 
glib2.0 introduced a change in 2.67.3 and later which triggers an
issue [1] for anyone including it's headers in a "extern C" context
which a few files in disas/* do. An example of such an include chain
and error look like:

../../disas/arm-a64.cc
In file included from /usr/include/glib-2.0/glib/gmacros.h:241,
                 from /usr/lib/x86_64-linux-gnu/glib-2.0/include/glibconfig.h:9,
                 from /usr/include/glib-2.0/glib/gtypes.h:32,
                 from /usr/include/glib-2.0/glib/galloca.h:32,
                 from /usr/include/glib-2.0/glib.h:30,
                 from /<<BUILDDIR>>/qemu-5.2+dfsg/include/glib-compat.h:32,
                 from /<<BUILDDIR>>/qemu-5.2+dfsg/include/qemu/osdep.h:126,
                 from ../../disas/arm-a64.cc:21:
/usr/include/c++/10/type_traits:56:3: error: template with C linkage
   56 |   template<typename _Tp, _Tp __v>
      |   ^~~~~~~~
../../disas/arm-a64.cc:20:1: note: ‘extern "C"’ linkage started here
   20 | extern "C" {
      | ^~~~~~~~~~

To fix that move the include of osdep.h out of that section. It was added
already as C++ fixup by e78490c44: "disas/arm-a64.cc: Include osdep.h first".

[1]: https://gitlab.gnome.org/GNOME/glib/-/issues/2331

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 disas/arm-a64.cc   | 2 +-
 disas/nanomips.cpp | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/disas/arm-a64.cc b/disas/arm-a64.cc
index 9fa779e175..27613d4b25 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 90e63b8367..3c202075cc 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.30.0
Re: [PATCH] disas: Fix build with glib2.0 >=2.67.3
Posted by Peter Maydell 4 years, 2 months ago 
On Tue, 23 Feb 2021 at 15:03, Christian Ehrhardt
<christian.ehrhardt@canonical.com> wrote:
>
> glib2.0 introduced a change in 2.67.3 and later which triggers an
> issue [1] for anyone including it's headers in a "extern C" context
> which a few files in disas/* do. An example of such an include chain
> and error look like:
>
> ../../disas/arm-a64.cc
> In file included from /usr/include/glib-2.0/glib/gmacros.h:241,
>                  from /usr/lib/x86_64-linux-gnu/glib-2.0/include/glibconfig.h:9,
>                  from /usr/include/glib-2.0/glib/gtypes.h:32,
>                  from /usr/include/glib-2.0/glib/galloca.h:32,
>                  from /usr/include/glib-2.0/glib.h:30,
>                  from /<<BUILDDIR>>/qemu-5.2+dfsg/include/glib-compat.h:32,
>                  from /<<BUILDDIR>>/qemu-5.2+dfsg/include/qemu/osdep.h:126,
>                  from ../../disas/arm-a64.cc:21:
> /usr/include/c++/10/type_traits:56:3: error: template with C linkage
>    56 |   template<typename _Tp, _Tp __v>
>       |   ^~~~~~~~
> ../../disas/arm-a64.cc:20:1: note: ‘extern "C"’ linkage started here
>    20 | extern "C" {
>       | ^~~~~~~~~~
>
> To fix that move the include of osdep.h out of that section. It was added
> already as C++ fixup by e78490c44: "disas/arm-a64.cc: Include osdep.h first".
>
> [1]: https://gitlab.gnome.org/GNOME/glib/-/issues/2331

I'm not convinced by this as a fix, though I'm happy to be corrected
by somebody with a fuller understanding of C++. glib.h may be supposed
to work as a C++ header, but osdep.h as a whole is definitely a C header,
so I think it ought to be inside 'extern C'; and it has to be
the first header included; and it happens to want to include glib.h.

Fixing glib.h seems like it would be nicer, assuming they haven't
already shipped this breakage. Failing that, does it work to do:

/*
 * glib.h expects to be included as a C++ header if we're
 * building a C++ file, but osdep.h and thus glib-compat.h are
 * C headers and should be inside an "extern C" block.
 */
#ifdef __cplusplus
extern "C++" {
#include <glib.h>
#if defined(G_OS_UNIX)
#include <glib-unix.h>
#endif
}

in include/glib-compat.h ?

thanks
-- PMM
Re: [PATCH] disas: Fix build with glib2.0 >=2.67.3
Posted by Daniel P. Berrangé 4 years, 2 months ago 
On Tue, Feb 23, 2021 at 03:43:48PM +0000, Peter Maydell wrote:
> On Tue, 23 Feb 2021 at 15:03, Christian Ehrhardt
> <christian.ehrhardt@canonical.com> wrote:
> >
> > glib2.0 introduced a change in 2.67.3 and later which triggers an
> > issue [1] for anyone including it's headers in a "extern C" context
> > which a few files in disas/* do. An example of such an include chain
> > and error look like:
> >
> > ../../disas/arm-a64.cc
> > In file included from /usr/include/glib-2.0/glib/gmacros.h:241,
> >                  from /usr/lib/x86_64-linux-gnu/glib-2.0/include/glibconfig.h:9,
> >                  from /usr/include/glib-2.0/glib/gtypes.h:32,
> >                  from /usr/include/glib-2.0/glib/galloca.h:32,
> >                  from /usr/include/glib-2.0/glib.h:30,
> >                  from /<<BUILDDIR>>/qemu-5.2+dfsg/include/glib-compat.h:32,
> >                  from /<<BUILDDIR>>/qemu-5.2+dfsg/include/qemu/osdep.h:126,
> >                  from ../../disas/arm-a64.cc:21:
> > /usr/include/c++/10/type_traits:56:3: error: template with C linkage
> >    56 |   template<typename _Tp, _Tp __v>
> >       |   ^~~~~~~~
> > ../../disas/arm-a64.cc:20:1: note: ‘extern "C"’ linkage started here
> >    20 | extern "C" {
> >       | ^~~~~~~~~~
> >
> > To fix that move the include of osdep.h out of that section. It was added
> > already as C++ fixup by e78490c44: "disas/arm-a64.cc: Include osdep.h first".
> >
> > [1]: https://gitlab.gnome.org/GNOME/glib/-/issues/2331
> 
> I'm not convinced by this as a fix, though I'm happy to be corrected
> by somebody with a fuller understanding of C++. glib.h may be supposed
> to work as a C++ header, but osdep.h as a whole is definitely a C header,
> so I think it ought to be inside 'extern C'; and it has to be
> the first header included; and it happens to want to include glib.h.
> 
> Fixing glib.h seems like it would be nicer, assuming they haven't
> already shipped this breakage. Failing that, does it work to do:

This was raised in Fedora and upstream GLib already

https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1935
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/J3P4TRHLWNDIKXF76OLYZNAPTABCZ3U5/#7LXFUDBBBIT23FE44QJYWX3I7U4EHW6M

The key comment there is this one:

  "Note that wrapping the header include in an extern declaration violates 
   C++ standard requirements.  ("A translation unit shall include a header 
   only outside of any declaration or definition", [using.headers]/3)"

IOW, if we need to make osdep.h safe to use from C++, then we
need to put the 'extern "C" { ... }'  bit in osdep.h itself,
not in the things which include it.

> 
> /*
>  * glib.h expects to be included as a C++ header if we're
>  * building a C++ file, but osdep.h and thus glib-compat.h are
>  * C headers and should be inside an "extern C" block.
>  */
> #ifdef __cplusplus
> extern "C++" {
> #include <glib.h>
> #if defined(G_OS_UNIX)
> #include <glib-unix.h>
> #endif
> }
> 
> in include/glib-compat.h ?

That'd be even worse.

We need to make headers that need to be used from C++ code follow
the pattern:

    #include <foo1>
    #include <foo2>
    #include <foo3>
    ...all other includs..

    extern "C" {
        ..
        only the declarations, no #includes
	...
    };



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] disas: Fix build with glib2.0 >=2.67.3
Posted by Christian Ehrhardt 4 years, 2 months ago 
On Tue, Feb 23, 2021 at 5:12 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Feb 23, 2021 at 03:43:48PM +0000, Peter Maydell wrote:
> > On Tue, 23 Feb 2021 at 15:03, Christian Ehrhardt
> > <christian.ehrhardt@canonical.com> wrote:
> > >
> > > glib2.0 introduced a change in 2.67.3 and later which triggers an
> > > issue [1] for anyone including it's headers in a "extern C" context
> > > which a few files in disas/* do. An example of such an include chain
> > > and error look like:
> > >
> > > ../../disas/arm-a64.cc
> > > In file included from /usr/include/glib-2.0/glib/gmacros.h:241,
> > >                  from /usr/lib/x86_64-linux-gnu/glib-2.0/include/glibconfig.h:9,
> > >                  from /usr/include/glib-2.0/glib/gtypes.h:32,
> > >                  from /usr/include/glib-2.0/glib/galloca.h:32,
> > >                  from /usr/include/glib-2.0/glib.h:30,
> > >                  from /<<BUILDDIR>>/qemu-5.2+dfsg/include/glib-compat.h:32,
> > >                  from /<<BUILDDIR>>/qemu-5.2+dfsg/include/qemu/osdep.h:126,
> > >                  from ../../disas/arm-a64.cc:21:
> > > /usr/include/c++/10/type_traits:56:3: error: template with C linkage
> > >    56 |   template<typename _Tp, _Tp __v>
> > >       |   ^~~~~~~~
> > > ../../disas/arm-a64.cc:20:1: note: ‘extern "C"’ linkage started here
> > >    20 | extern "C" {
> > >       | ^~~~~~~~~~
> > >
> > > To fix that move the include of osdep.h out of that section. It was added
> > > already as C++ fixup by e78490c44: "disas/arm-a64.cc: Include osdep.h first".
> > >
> > > [1]: https://gitlab.gnome.org/GNOME/glib/-/issues/2331
> >
> > I'm not convinced by this as a fix, though I'm happy to be corrected
> > by somebody with a fuller understanding of C++. glib.h may be supposed
> > to work as a C++ header, but osdep.h as a whole is definitely a C header,
> > so I think it ought to be inside 'extern C'; and it has to be
> > the first header included; and it happens to want to include glib.h.
> >
> > Fixing glib.h seems like it would be nicer, assuming they haven't
> > already shipped this breakage.

They have already shipped this. And in the linked issue and from there
MR discussions
you'll find that everyone acknowledges that the "error" is in the
consuming projects
and that glib upstream is rejecting to fix it for e.g. the argument of
backward compatibility.

> Failing that, does it work to do:
>
> This was raised in Fedora and upstream GLib already
>
> https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1935
> https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/J3P4TRHLWNDIKXF76OLYZNAPTABCZ3U5/#7LXFUDBBBIT23FE44QJYWX3I7U4EHW6M
>
> The key comment there is this one:
>
>   "Note that wrapping the header include in an extern declaration violates
>    C++ standard requirements.  ("A translation unit shall include a header
>    only outside of any declaration or definition", [using.headers]/3)"
>
> IOW, if we need to make osdep.h safe to use from C++, then we
> need to put the 'extern "C" { ... }'  bit in osdep.h itself,
> not in the things which include it.
>
> >
> > /*
> >  * glib.h expects to be included as a C++ header if we're
> >  * building a C++ file, but osdep.h and thus glib-compat.h are
> >  * C headers and should be inside an "extern C" block.
> >  */
> > #ifdef __cplusplus
> > extern "C++" {
> > #include <glib.h>
> > #if defined(G_OS_UNIX)
> > #include <glib-unix.h>
> > #endif
> > }
> >
> > in include/glib-compat.h ?
>
> That'd be even worse.
>
> We need to make headers that need to be used from C++ code follow
> the pattern:
>
>     #include <foo1>
>     #include <foo2>
>     #include <foo3>
>     ...all other includs..
>
>     extern "C" {
>         ..
>         only the declarations, no #includes
>         ...
>     };

While I can follow the words and always awesome explanations by Daniel,
I must admit that I'm a bit lost at what a v2 of this could look like.

osdep.h as of today unfortunately isn't as trivial as 1. include 2.
declarations.
There are late includes deep in cascading ifdef's and we all know that "just
moving includes around for the above fix to work in an easy way" in headers
will likely (maybe even silently) break things.

So I wonder is this going to become a massive patch either moving a lot or
adding many extern C declarations all over the place in osdep-h? Or did I
just fail to see that there is an obviously better approach to this?

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


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.