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

[Daniel P . Berrangé]

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