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" +

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

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?

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.

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(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. 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.

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(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. > > > > 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.