On Mon, Nov 16, 2020 at 10:52:54AM +0100, Martin Kletzander wrote:
> On Mon, Nov 16, 2020 at 09:24:22AM +0000, Daniel P. Berrangé wrote:
> > On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote:
> > > This reverts commit b3710e9a2af402a2b620de570b062294e11190eb.
> > >
> > > That check is very valuable for our code, but it causes issue with glib >=
> > > 2.67.0 when building with clang.
> > >
> > > The reason is a combination of two commits in glib, firstly fdda405b6b1b which
> > > adds a g_atomic_pointer_{set,get} variants that enforce stricter type
> > > checking (by removing an extra cast) for compilers that support __typeof__, and
> > > commit dce24dc4492d which effectively enabled the new variant of glib's atomic
> > > code for clang. This will not be necessary when glib's issue #600 [0] (8 years
> > > old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this
> > > issue was opened 3 weeks ago, so there is a slight sliver of hope.
> > >
> > > [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600
> > > [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719
> > >
> > > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > > ---
> > > meson.build | 3 ---
> > > src/qemu/qemu_domain.c | 7 +++++++
> > > src/util/vireventthread.c | 6 ++++++
> > > src/util/viridentity.c | 6 ++++++
> > > src/util/virobject.c | 6 ++++++
> > > 5 files changed, 25 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/meson.build b/meson.build
> > > index cecaad199d4c..04646e3a078c 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -405,9 +405,6 @@ cc_flags += [
> > > # so use this Clang-specific arg to keep it quiet
> > > '-Wno-typedef-redefinition',
> > >
> > > - # Clang complains about casts in G_DEFINE_TYPE(...)
> > > - '-Wno-incompatible-pointer-types-discards-qualifiers',
> > > -
> > > # We don't use -Wc++-compat so we have to enable it explicitly
> > > '-Wjump-misses-init',
> > >
> > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > index 2158080a56ad..3349476cceae 100644
> > > --- a/src/qemu/qemu_domain.c
> > > +++ b/src/qemu/qemu_domain.c
> > > @@ -330,7 +330,14 @@ struct _qemuDomainLogContext {
> > > virLogManagerPtr manager;
> > > };
> > >
> > > +#pragma clang diagnostic push
> > > +/* Workaround for glib's issue #600 on clang */
> > > +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers"
> > > +
> > > G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT);
> > > +
> > > +#pragma clang diagnostic pop
> >
> > I really don't want to see this pattern used. We may only have a handful
> > of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject
> > this is going to be way to tedious. This has to be done as a global
> > thing IMHO.
> >
>
> Even when the warning we're disabling actually catches errors we make every now
> and then? Really?
You can disable it globall, but only when compiling against the glib versions
affected. We'll still get coverage of the warning flag when building against
other versions.
>
> I know the issue is very old and it only showed up now. And that the MR might
> not go through for quite some time. But how about a middle ground like the one
> I described in reply to Pavel? Looks like this:
>
> #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \
> _Pragma ("clang diagnostic push") \
> _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \
> G_DEFINE_TYPE(a, b, c) \
> _Pragma ("clang diagnostic pop")
>
> If someone wants to make sure it does not break and we really want to get rid of
> this ASAP, then we can even syntax-check for it, describe it and only use it
> with glib that has this issue.
That can't be done selectively - we would have to use that wrapper macro
unconditionally, so it'd be present for years until we bump the min glib
to a version that has the fix.
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 :|
