[libvirt PATCH] Do not disable incompatible-pointer-types-discards-qualifiers
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 + static virClassPtr qemuDomainSaveCookieClass; static void qemuDomainLogContextFinalize(GObject *obj); diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c index 8342f420f666..4e202c94d0f0 100644 --- a/src/util/vireventthread.c +++ b/src/util/vireventthread.c @@ -32,8 +32,14 @@ struct _virEventThread { GMainLoop *loop; }; +#pragma clang diagnostic push +/* Workaround for glib's issue #600 on clang */ +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" + G_DEFINE_TYPE(virEventThread, vir_event_thread, G_TYPE_OBJECT) +#pragma clang diagnostic pop + #define VIR_FROM_THIS VIR_FROM_EVENT static void diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 2cb9042a846a..43f447f629fe 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -50,8 +50,14 @@ struct _virIdentity { virTypedParameterPtr params; }; +#pragma clang diagnostic push +/* Workaround for glib's issue #600 on clang */ +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" + G_DEFINE_TYPE(virIdentity, vir_identity, G_TYPE_OBJECT) +#pragma clang diagnostic pop + static virThreadLocal virIdentityCurrent; static void virIdentityFinalize(GObject *obj); diff --git a/src/util/virobject.c b/src/util/virobject.c index a6305354c35e..8d1f26618334 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -53,8 +53,14 @@ struct _virObjectPrivate { }; +#pragma clang diagnostic push +/* Workaround for glib's issue #600 on clang */ +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" + G_DEFINE_TYPE_WITH_PRIVATE(virObject, vir_object, G_TYPE_OBJECT) +#pragma clang diagnostic pop + #define VIR_OBJECT_NOTVALID(obj) (!obj || !VIR_IS_OBJECT(obj)) #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ -- 2.29.2
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" +
Instead of repeating these two macros you can create our own in src/internal.h like we do for other ignores. Otherwise looks good. Pavel
On Mon, Nov 16, 2020 at 09:21:23AM +0100, Pavel Hrdina 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" +
Instead of repeating these two macros you can create our own in src/internal.h like we do for other ignores.
Oh, I did not know about using _Pragma in macros. I initialy wanted to make a wrapper macro for the G_DEFINE_TYPE itself, something like: #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") But I'm not sure this is something that would not go against other people's ideas.
Otherwise looks good.
Pavel
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. 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 :|
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? 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.
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 :|
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. 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 :|
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. Michal
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. 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 :|
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 :|
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@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.
No code ever calls vir_g_canonicalize_filename directly, it continues to use g_canonicalize_filename as normal. The calls get transparently re-written to vir_g_canonicalize_filename - basically the same hack that gnulib used to transparently replace functions. 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 :|
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@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 :|
participants (4)
-
Daniel P. Berrangé -
Martin Kletzander -
Michal Privoznik -
Pavel Hrdina