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

Martin Kletzander posted 1 patch 1 week ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/dd896750d75e084adab2bc5d044ac6e412dd99d6.1605558583.git.mkletzan@redhat.com
meson.build           |  3 ---
src/util/glibcompat.h | 24 ++++++++++++++++++++++++
2 files changed, 24 insertions(+), 3 deletions(-)

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

Posted by Martin Kletzander 1 week 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>
---
v3:
 - added check for glib version

v2:
 - override _G_DEFINE_TYPE_EXTENDED_BEGIN in gcompat.h

 meson.build           |  3 ---
 src/util/glibcompat.h | 24 ++++++++++++++++++++++++
 2 files changed, 24 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/util/glibcompat.h b/src/util/glibcompat.h
index 6f50a76f3c2b..457f6ba797f3 100644
--- a/src/util/glibcompat.h
+++ b/src/util/glibcompat.h
@@ -20,6 +20,30 @@
 
 #include <glib.h>
 #include <glib/gstdio.h>
+#include <glib-object.h>
+
+#if defined(__clang__) && GLIB_CHECK_VERSION(2, 67, 0)
+/*
+ * Clang detects (valid) issue in G_DEFINE_TYPE and derivatives starting with
+ * glib >= 2.67.0.  See https://gitlab.gnome.org/GNOME/glib/-/issues/600
+ *
+ * For that we need to disable the one check that produces an error in our
+ * builds when using any G_DEFINE_TYPE* macro.  Thankfully all those macros end
+ * up using _G_DEFINE_TYPE_EXTENDED_BEGIN.  Because with that we can redefine
+ * this one macro to cover all use cases.  The macro is defined the same way it
+ * is defined in glib (with a very low probability of being changed thanks to a
+ * comment above it).
+ */
+# undef _G_DEFINE_TYPE_EXTENDED_BEGIN
+
+# define _G_DEFINE_TYPE_EXTENDED_BEGIN(TypeName, type_name, TYPE_PARENT, flags) \
+    _Pragma("GCC diagnostic push") \
+    _Pragma("GCC diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \
+    _G_DEFINE_TYPE_EXTENDED_BEGIN_PRE(TypeName, type_name, TYPE_PARENT) \
+    _G_DEFINE_TYPE_EXTENDED_BEGIN_REGISTER(TypeName, type_name, TYPE_PARENT, flags) \
+    _Pragma("GCC diagnostic pop")
+
+#endif /* __clang__ */
 
 gchar * vir_g_canonicalize_filename(const gchar *filename,
                                     const gchar *relative_to);
-- 
2.29.2

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

Posted by Daniel P. Berrangé 6 days, 15 hours ago
On Mon, Nov 16, 2020 at 09:31:22PM +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>
> ---
> v3:
>  - added check for glib version
> 
> v2:
>  - override _G_DEFINE_TYPE_EXTENDED_BEGIN in gcompat.h
> 
>  meson.build           |  3 ---
>  src/util/glibcompat.h | 24 ++++++++++++++++++++++++
>  2 files changed, 24 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/util/glibcompat.h b/src/util/glibcompat.h
> index 6f50a76f3c2b..457f6ba797f3 100644
> --- a/src/util/glibcompat.h
> +++ b/src/util/glibcompat.h
> @@ -20,6 +20,30 @@
>  
>  #include <glib.h>
>  #include <glib/gstdio.h>
> +#include <glib-object.h>
> +
> +#if defined(__clang__) && GLIB_CHECK_VERSION(2, 67, 0)
> +/*
> + * Clang detects (valid) issue in G_DEFINE_TYPE and derivatives starting with
> + * glib >= 2.67.0.  See https://gitlab.gnome.org/GNOME/glib/-/issues/600
> + *
> + * For that we need to disable the one check that produces an error in our
> + * builds when using any G_DEFINE_TYPE* macro.  Thankfully all those macros end
> + * up using _G_DEFINE_TYPE_EXTENDED_BEGIN.  Because with that we can redefine
> + * this one macro to cover all use cases.  The macro is defined the same way it
> + * is defined in glib (with a very low probability of being changed thanks to a
> + * comment above it).
> + */
> +# undef _G_DEFINE_TYPE_EXTENDED_BEGIN
> +
> +# define _G_DEFINE_TYPE_EXTENDED_BEGIN(TypeName, type_name, TYPE_PARENT, flags) \
> +    _Pragma("GCC diagnostic push") \
> +    _Pragma("GCC diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \
> +    _G_DEFINE_TYPE_EXTENDED_BEGIN_PRE(TypeName, type_name, TYPE_PARENT) \
> +    _G_DEFINE_TYPE_EXTENDED_BEGIN_REGISTER(TypeName, type_name, TYPE_PARENT, flags) \
> +    _Pragma("GCC diagnostic pop")
> +
> +#endif /* __clang__ */
>  
>  gchar * vir_g_canonicalize_filename(const gchar *filename,
>                                      const gchar *relative_to);

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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