[libvirt PATCH] Do not disable incompatible-pointer-types-discards-qualifiers

Mon Nov 16 11:50:27 UTC 2020

On Mon, Nov 16, 2020 at 12:45:41PM +0100, Martin Kletzander wrote:
>On Mon, Nov 16, 2020 at 10:48:20AM +0000, Daniel P. Berrangé wrote:
>>On Mon, Nov 16, 2020 at 11:26:45AM +0100, Michal Privoznik wrote:
>>> On 11/16/20 10:59 AM, Daniel P. Berrangé wrote:
>>> > 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 at redhat.com>
>>> > > > > ---
>>> > > > >   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.
>>>
>>> So what about something like this:
>>>
>>> #if glib is broken
>>> # undefine G_DEFINE_TYPE
>>> # define G_DEFINE_TYPE \
>>> #   some pragma magic (basically what martin has above) \
>>> #endif
>>>
>>> This way we can continue using G_DEFINE_TYPE and ditch this redefine once
>>> glib is updated to fixed version.
>>
>>If you can make that work, that'd be ok as it isolates the workaround in
>>a single place.
>>
>
>OK, thanks Michal for following up because I was kind of misunderstanding the
>intention.
>
>If we #undef the original G_DEFINE_TYPE we would have to open code it, being
>prone to errors if the original definition changes.  If we change it to
>e.g. VIR_G_DEFINE_TYPE() (and disable G_DEFINE_TYPE usage out of
>sr/util/glibcompat.c) then we can just do what I wrote above and whenever it is
>removed it is easy to see what places in the code need changing.  I know you are
>against that, but I don't see how different that is compared to, for example
>vir_g_canonicalize_filename().  And other functions in gcompat.h also, although
>they don't even have the version check.
>

OK, my bad, gcompat.h makes all the difference.  I'll see what I can come up with.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201116/aeea32c9/attachment-0001.sig>