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

[Michal Privoznik]

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 at 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