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