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

Martin Kletzander posted 1 patch 1 week, 1 day ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1cb89bfc8bf7f60bc86093de6fa4c8f6a54408f7.1605472909.git.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(-)

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

Posted by Martin Kletzander 1 week, 1 day ago
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

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

Posted by Daniel P. Berrangé 1 week ago
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 :|

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

Posted by Martin Kletzander 1 week ago
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 :|

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

Posted by Daniel P. Berrangé 1 week ago
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 :|

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

Posted by Michal Privoznik 1 week ago
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

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

Posted by Daniel P. Berrangé 1 week ago
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 :|

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

Posted by Martin Kletzander 1 week ago
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 :|

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

Posted by Martin Kletzander 1 week ago
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 :|


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

Posted by Daniel P. Berrangé 1 week ago
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 :|

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

Posted by Pavel Hrdina 1 week ago
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

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

Posted by Martin Kletzander 1 week ago
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