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

[Christian Ehrhardt]

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