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