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