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

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(a)redhat.com&gt; >>> --- >>> 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.