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