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

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

[Christian Ehrhardt]

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

[Peter Maydell]

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

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

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

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

[Daniel P. Berrangé]

On Wed, Feb 24, 2021 at 08:38:30AM +0100, Christian Ehrhardt wrote:
> 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:
> >
> > 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?

I don't think it will need reordering osdep.h.

Most of the #include are system headers, which should already be
protected by 'extern "C"' themselves if needed.


So from osdep.h I think something like this is likely sufficient:

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ba15be9c56..7a1d83a8b6 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -126,6 +126,7 @@ extern int daemon(int, int);
 #include "glib-compat.h"
 #include "qemu/typedefs.h"
 
+extern "C" {
 /*
  * For mingw, as of v6.0.0, the function implementing the assert macro is
  * not marked as noreturn, so the compiler cannot delete code following an
@@ -722,4 +723,6 @@ static inline int platform_does_not_support_system(const char *command)
 }
 #endif /* !HAVE_SYSTEM_FUNCTION */
 
+}
+
 #endif


We'll also need to them protect any local headers we use before this point.

$ grep #include ../../../include/qemu/osdep.h  | grep -v '<'
#include "config-host.h"
#include CONFIG_TARGET
#include "exec/poison.h"
#include "qemu/compiler.h"
#include "sysemu/os-win32.h"
#include "sysemu/os-posix.h"
#include "glib-compat.h"
#include "qemu/typedefs.h"

and transitively through that list, but I think there's no too many
more there.

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

[Peter Maydell]

On Wed, 24 Feb 2021 at 11:04, Daniel P. Berrangé <berrange@redhat.com> wrote:
> So from osdep.h I think something like this is likely sufficient:
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index ba15be9c56..7a1d83a8b6 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -126,6 +126,7 @@ extern int daemon(int, int);
>  #include "glib-compat.h"
>  #include "qemu/typedefs.h"
>
> +extern "C" {

Needs to be protected by #ifdef so it's only relevant for the
C++ compiler, right?

>  /*
>   * For mingw, as of v6.0.0, the function implementing the assert macro is
>   * not marked as noreturn, so the compiler cannot delete code following an
> @@ -722,4 +723,6 @@ static inline int platform_does_not_support_system(const char *command)
>  }
>  #endif /* !HAVE_SYSTEM_FUNCTION */
>
> +}
> +
>  #endif
>
>
> We'll also need to them protect any local headers we use before this point.
>
> $ grep #include ../../../include/qemu/osdep.h  | grep -v '<'
> #include "config-host.h"
> #include CONFIG_TARGET
> #include "exec/poison.h"
> #include "qemu/compiler.h"
> #include "sysemu/os-win32.h"
> #include "sysemu/os-posix.h"
> #include "glib-compat.h"
> #include "qemu/typedefs.h"
>
> and transitively through that list, but I think there's no too many
> more there.

Is there anything we can do to make the compiler complain if we
get this wrong? Otherwise it seems likely that we'll end up
accidentally putting things inside or outside 'extern "C"'
declarations when they shouldn't be, as we make future changes
to our headers.

(The other approach would be to try to get rid of the
C++ in the codebase. We could probably say 'drop vixl
and always use capstone', for instance.)

thanks
-- PMM

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

[Daniel P. Berrangé]

On Wed, Feb 24, 2021 at 01:07:33PM +0000, Peter Maydell wrote:
> On Wed, 24 Feb 2021 at 11:04, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > So from osdep.h I think something like this is likely sufficient:
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index ba15be9c56..7a1d83a8b6 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -126,6 +126,7 @@ extern int daemon(int, int);
> >  #include "glib-compat.h"
> >  #include "qemu/typedefs.h"
> >
> > +extern "C" {
> 
> Needs to be protected by #ifdef so it's only relevant for the
> C++ compiler, right?
> 
> >  /*
> >   * For mingw, as of v6.0.0, the function implementing the assert macro is
> >   * not marked as noreturn, so the compiler cannot delete code following an
> > @@ -722,4 +723,6 @@ static inline int platform_does_not_support_system(const char *command)
> >  }
> >  #endif /* !HAVE_SYSTEM_FUNCTION */
> >
> > +}
> > +
> >  #endif
> >
> >
> > We'll also need to them protect any local headers we use before this point.
> >
> > $ grep #include ../../../include/qemu/osdep.h  | grep -v '<'
> > #include "config-host.h"
> > #include CONFIG_TARGET
> > #include "exec/poison.h"
> > #include "qemu/compiler.h"
> > #include "sysemu/os-win32.h"
> > #include "sysemu/os-posix.h"
> > #include "glib-compat.h"
> > #include "qemu/typedefs.h"
> >
> > and transitively through that list, but I think there's no too many
> > more there.
> 
> Is there anything we can do to make the compiler complain if we
> get this wrong? Otherwise it seems likely that we'll end up
> accidentally putting things inside or outside 'extern "C"'
> declarations when they shouldn't be, as we make future changes
> to our headers.

There's nothing easy I know of to highlight this.  It is more the kind
of thing checkpatch would have to look at - complain if there is
anything which isn't a  preprocessor include directive or comment
before the 'extern'.

> (The other approach would be to try to get rid of the
> C++ in the codebase. We could probably say 'drop vixl
> and always use capstone', for instance.)

Yeah, getting rid of C++ would probably be the sanest solution long
term.

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

[Christian Ehrhardt]

On Wed, Feb 24, 2021 at 2:15 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Feb 24, 2021 at 01:07:33PM +0000, Peter Maydell wrote:
> > On Wed, 24 Feb 2021 at 11:04, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > So from osdep.h I think something like this is likely sufficient:
> > >
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index ba15be9c56..7a1d83a8b6 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -126,6 +126,7 @@ extern int daemon(int, int);
> > >  #include "glib-compat.h"
> > >  #include "qemu/typedefs.h"
> > >
> > > +extern "C" {
> >
> > Needs to be protected by #ifdef so it's only relevant for the
> > C++ compiler, right?
> >
> > >  /*
> > >   * For mingw, as of v6.0.0, the function implementing the assert macro is
> > >   * not marked as noreturn, so the compiler cannot delete code following an
> > > @@ -722,4 +723,6 @@ static inline int platform_does_not_support_system(const char *command)
> > >  }
> > >  #endif /* !HAVE_SYSTEM_FUNCTION */
> > >
> > > +}
> > > +
> > >  #endif
> > >
> > >
> > > We'll also need to them protect any local headers we use before this point.
> > >
> > > $ grep #include ../../../include/qemu/osdep.h  | grep -v '<'
> > > #include "config-host.h"
> > > #include CONFIG_TARGET
> > > #include "exec/poison.h"
> > > #include "qemu/compiler.h"
> > > #include "sysemu/os-win32.h"
> > > #include "sysemu/os-posix.h"
> > > #include "glib-compat.h"
> > > #include "qemu/typedefs.h"
> > >
> > > and transitively through that list, but I think there's no too many
> > > more there.
> >
> > Is there anything we can do to make the compiler complain if we
> > get this wrong? Otherwise it seems likely that we'll end up
> > accidentally putting things inside or outside 'extern "C"'
> > declarations when they shouldn't be, as we make future changes
> > to our headers.
>
> There's nothing easy I know of to highlight this.  It is more the kind
> of thing checkpatch would have to look at - complain if there is
> anything which isn't a  preprocessor include directive or comment
> before the 'extern'.
>
> > (The other approach would be to try to get rid of the
> > C++ in the codebase. We could probably say 'drop vixl
> > and always use capstone', for instance.)
>
> Yeah, getting rid of C++ would probably be the sanest solution long
> term.

Just as an input on short-term alternatives,
in open-vm-tools we've found to follow
https://developer.gnome.org/glib/stable/glib-Version-Information.html#GLIB-VERSION-MIN-REQUIRED:CAPS
to be an easy fix for the time being.
Which in the autoconf magic there was just:
  +AC_DEFINE(GLIB_VERSION_MIN_REQUIRED, GLIB_VERSION_2_34, [Ignore
post 2.34 deprecations])
  +AC_DEFINE(GLIB_VERSION_MAX_ALLOWED, GLIB_VERSION_2_34, [Prevent
post 2.34 APIs])
(Or any other/newer version that one would want to select)

Not sure what would apply to qemu here, but I wanted to let you know
of the overall concept in regard to this issue.


> 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

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

[Daniel P. Berrangé]

On Mon, Mar 08, 2021 at 02:50:39PM +0100, Christian Ehrhardt wrote:
> On Wed, Feb 24, 2021 at 2:15 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Feb 24, 2021 at 01:07:33PM +0000, Peter Maydell wrote:
> > > On Wed, 24 Feb 2021 at 11:04, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > So from osdep.h I think something like this is likely sufficient:
> > > >
> > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > index ba15be9c56..7a1d83a8b6 100644
> > > > --- a/include/qemu/osdep.h
> > > > +++ b/include/qemu/osdep.h
> > > > @@ -126,6 +126,7 @@ extern int daemon(int, int);
> > > >  #include "glib-compat.h"
> > > >  #include "qemu/typedefs.h"
> > > >
> > > > +extern "C" {
> > >
> > > Needs to be protected by #ifdef so it's only relevant for the
> > > C++ compiler, right?
> > >
> > > >  /*
> > > >   * For mingw, as of v6.0.0, the function implementing the assert macro is
> > > >   * not marked as noreturn, so the compiler cannot delete code following an
> > > > @@ -722,4 +723,6 @@ static inline int platform_does_not_support_system(const char *command)
> > > >  }
> > > >  #endif /* !HAVE_SYSTEM_FUNCTION */
> > > >
> > > > +}
> > > > +
> > > >  #endif
> > > >
> > > >
> > > > We'll also need to them protect any local headers we use before this point.
> > > >
> > > > $ grep #include ../../../include/qemu/osdep.h  | grep -v '<'
> > > > #include "config-host.h"
> > > > #include CONFIG_TARGET
> > > > #include "exec/poison.h"
> > > > #include "qemu/compiler.h"
> > > > #include "sysemu/os-win32.h"
> > > > #include "sysemu/os-posix.h"
> > > > #include "glib-compat.h"
> > > > #include "qemu/typedefs.h"
> > > >
> > > > and transitively through that list, but I think there's no too many
> > > > more there.
> > >
> > > Is there anything we can do to make the compiler complain if we
> > > get this wrong? Otherwise it seems likely that we'll end up
> > > accidentally putting things inside or outside 'extern "C"'
> > > declarations when they shouldn't be, as we make future changes
> > > to our headers.
> >
> > There's nothing easy I know of to highlight this.  It is more the kind
> > of thing checkpatch would have to look at - complain if there is
> > anything which isn't a  preprocessor include directive or comment
> > before the 'extern'.
> >
> > > (The other approach would be to try to get rid of the
> > > C++ in the codebase. We could probably say 'drop vixl
> > > and always use capstone', for instance.)
> >
> > Yeah, getting rid of C++ would probably be the sanest solution long
> > term.
> 
> Just as an input on short-term alternatives,
> in open-vm-tools we've found to follow
> https://developer.gnome.org/glib/stable/glib-Version-Information.html#GLIB-VERSION-MIN-REQUIRED:CAPS
> to be an easy fix for the time being.
> Which in the autoconf magic there was just:
>   +AC_DEFINE(GLIB_VERSION_MIN_REQUIRED, GLIB_VERSION_2_34, [Ignore
> post 2.34 deprecations])
>   +AC_DEFINE(GLIB_VERSION_MAX_ALLOWED, GLIB_VERSION_2_34, [Prevent
> post 2.34 APIs])
> (Or any other/newer version that one would want to select)
> 
> Not sure what would apply to qemu here, but I wanted to let you know
> of the overall concept in regard to this issue.

QEMU already uses these macros but they can't protect against all
scenarios. Principally they avoid you accidentally using a newly
introduced public API.  The scenario in this thread doesn't involve
a new API, so those macros don't help here, you need to actually
compile against the new version to see the problem

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