[Qemu-devel] [PATCH] qemu/compiler: Wrap __attribute__((flatten)) in a macro

Thomas Huth posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1537977586-21093-1-git-send-email-thuth@redhat.com
Test docker-clang@ubuntu failed
Test checkpatch failed
There is a newer version of this series
fpu/softfloat.c         | 30 +++++++++++++++---------------
include/qemu/compiler.h | 15 +++++++++++++++
2 files changed, 30 insertions(+), 15 deletions(-)
[Qemu-devel] [PATCH] qemu/compiler: Wrap __attribute__((flatten)) in a macro
Posted by Thomas Huth 5 years, 7 months ago
Older versions of Clang (before 3.5) and GCC (before 4.1) do not
support the "__attribute__((flatten))" yet. Since at least Clang
3.4 is still used in EPEL for RHEL7 / CentOS 7, we should not
use this attribute directly but with a wrapper macro instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 fpu/softfloat.c         | 30 +++++++++++++++---------------
 include/qemu/compiler.h | 15 +++++++++++++++
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 59ca356..b75ca07 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -726,7 +726,7 @@ static FloatParts addsub_floats(FloatParts a, FloatParts b, bool subtract,
  * IEC/IEEE Standard for Binary Floating-Point Arithmetic.
  */
 
-float16  __attribute__((flatten)) float16_add(float16 a, float16 b,
+float16 QEMU_FLATTEN float16_add(float16 a, float16 b,
                                               float_status *status)
 {
     FloatParts pa = float16_unpack_canonical(a, status);
@@ -736,7 +736,7 @@ float16  __attribute__((flatten)) float16_add(float16 a, float16 b,
     return float16_round_pack_canonical(pr, status);
 }
 
-float32 __attribute__((flatten)) float32_add(float32 a, float32 b,
+float32 QEMU_FLATTEN float32_add(float32 a, float32 b,
                                              float_status *status)
 {
     FloatParts pa = float32_unpack_canonical(a, status);
@@ -746,7 +746,7 @@ float32 __attribute__((flatten)) float32_add(float32 a, float32 b,
     return float32_round_pack_canonical(pr, status);
 }
 
-float64 __attribute__((flatten)) float64_add(float64 a, float64 b,
+float64 QEMU_FLATTEN float64_add(float64 a, float64 b,
                                              float_status *status)
 {
     FloatParts pa = float64_unpack_canonical(a, status);
@@ -756,7 +756,7 @@ float64 __attribute__((flatten)) float64_add(float64 a, float64 b,
     return float64_round_pack_canonical(pr, status);
 }
 
-float16 __attribute__((flatten)) float16_sub(float16 a, float16 b,
+float16 QEMU_FLATTEN float16_sub(float16 a, float16 b,
                                              float_status *status)
 {
     FloatParts pa = float16_unpack_canonical(a, status);
@@ -766,7 +766,7 @@ float16 __attribute__((flatten)) float16_sub(float16 a, float16 b,
     return float16_round_pack_canonical(pr, status);
 }
 
-float32 __attribute__((flatten)) float32_sub(float32 a, float32 b,
+float32 QEMU_FLATTEN float32_sub(float32 a, float32 b,
                                              float_status *status)
 {
     FloatParts pa = float32_unpack_canonical(a, status);
@@ -776,7 +776,7 @@ float32 __attribute__((flatten)) float32_sub(float32 a, float32 b,
     return float32_round_pack_canonical(pr, status);
 }
 
-float64 __attribute__((flatten)) float64_sub(float64 a, float64 b,
+float64 QEMU_FLATTEN float64_sub(float64 a, float64 b,
                                              float_status *status)
 {
     FloatParts pa = float64_unpack_canonical(a, status);
@@ -835,7 +835,7 @@ static FloatParts mul_floats(FloatParts a, FloatParts b, float_status *s)
     g_assert_not_reached();
 }
 
-float16 __attribute__((flatten)) float16_mul(float16 a, float16 b,
+float16 QEMU_FLATTEN float16_mul(float16 a, float16 b,
                                              float_status *status)
 {
     FloatParts pa = float16_unpack_canonical(a, status);
@@ -845,7 +845,7 @@ float16 __attribute__((flatten)) float16_mul(float16 a, float16 b,
     return float16_round_pack_canonical(pr, status);
 }
 
-float32 __attribute__((flatten)) float32_mul(float32 a, float32 b,
+float32 QEMU_FLATTEN float32_mul(float32 a, float32 b,
                                              float_status *status)
 {
     FloatParts pa = float32_unpack_canonical(a, status);
@@ -855,7 +855,7 @@ float32 __attribute__((flatten)) float32_mul(float32 a, float32 b,
     return float32_round_pack_canonical(pr, status);
 }
 
-float64 __attribute__((flatten)) float64_mul(float64 a, float64 b,
+float64 QEMU_FLATTEN float64_mul(float64 a, float64 b,
                                              float_status *status)
 {
     FloatParts pa = float64_unpack_canonical(a, status);
@@ -1068,7 +1068,7 @@ static FloatParts muladd_floats(FloatParts a, FloatParts b, FloatParts c,
     return a;
 }
 
-float16 __attribute__((flatten)) float16_muladd(float16 a, float16 b, float16 c,
+float16 QEMU_FLATTEN float16_muladd(float16 a, float16 b, float16 c,
                                                 int flags, float_status *status)
 {
     FloatParts pa = float16_unpack_canonical(a, status);
@@ -1079,7 +1079,7 @@ float16 __attribute__((flatten)) float16_muladd(float16 a, float16 b, float16 c,
     return float16_round_pack_canonical(pr, status);
 }
 
-float32 __attribute__((flatten)) float32_muladd(float32 a, float32 b, float32 c,
+float32 QEMU_FLATTEN float32_muladd(float32 a, float32 b, float32 c,
                                                 int flags, float_status *status)
 {
     FloatParts pa = float32_unpack_canonical(a, status);
@@ -1090,7 +1090,7 @@ float32 __attribute__((flatten)) float32_muladd(float32 a, float32 b, float32 c,
     return float32_round_pack_canonical(pr, status);
 }
 
-float64 __attribute__((flatten)) float64_muladd(float64 a, float64 b, float64 c,
+float64 QEMU_FLATTEN float64_muladd(float64 a, float64 b, float64 c,
                                                 int flags, float_status *status)
 {
     FloatParts pa = float64_unpack_canonical(a, status);
@@ -2402,21 +2402,21 @@ static FloatParts sqrt_float(FloatParts a, float_status *s, const FloatFmt *p)
     return a;
 }
 
-float16 __attribute__((flatten)) float16_sqrt(float16 a, float_status *status)
+float16 QEMU_FLATTEN float16_sqrt(float16 a, float_status *status)
 {
     FloatParts pa = float16_unpack_canonical(a, status);
     FloatParts pr = sqrt_float(pa, status, &float16_params);
     return float16_round_pack_canonical(pr, status);
 }
 
-float32 __attribute__((flatten)) float32_sqrt(float32 a, float_status *status)
+float32 QEMU_FLATTEN float32_sqrt(float32 a, float_status *status)
 {
     FloatParts pa = float32_unpack_canonical(a, status);
     FloatParts pr = sqrt_float(pa, status, &float32_params);
     return float32_round_pack_canonical(pr, status);
 }
 
-float64 __attribute__((flatten)) float64_sqrt(float64 a, float_status *status)
+float64 QEMU_FLATTEN float64_sqrt(float64 a, float_status *status)
 {
     FloatParts pa = float64_unpack_canonical(a, status);
     FloatParts pr = sqrt_float(pa, status, &float64_params);
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 5843812..7753ee8 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -119,6 +119,21 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+/*
+ * Clang 3.4 claims to be compatible with GCC 4.2, but does not have the
+ * "flatten" attribute, so we've got to handle Clang via __has_attribute here
+ */
+#if defined(__clang__) && defined(__has_attribute)
+# if __has_attribute(flatten)
+#  define QEMU_FLATTEN __attribute__((flatten))
+# endif
+#elif !defined(__clang__) && QEMU_GNUC_PREREQ(4, 1)
+# define QEMU_FLATTEN __attribute__((flatten))
+#endif
+#ifndef QEMU_FLATTEN
+# define QEMU_FLATTEN
+#endif
+
 #ifndef __has_feature
 #define __has_feature(x) 0 /* compatibility with non-clang compilers */
 #endif
-- 
1.8.3.1


Re: [Qemu-devel] [PATCH] qemu/compiler: Wrap __attribute__((flatten)) in a macro
Posted by Richard Henderson 5 years, 7 months ago
On 9/26/18 8:59 AM, Thomas Huth wrote:
> Older versions of Clang (before 3.5) and GCC (before 4.1) do not
> support the "__attribute__((flatten))" yet. Since at least Clang
> 3.4 is still used in EPEL for RHEL7 / CentOS 7, we should not
> use this attribute directly but with a wrapper macro instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  fpu/softfloat.c         | 30 +++++++++++++++---------------
>  include/qemu/compiler.h | 15 +++++++++++++++
>  2 files changed, 30 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH] qemu/compiler: Wrap __attribute__((flatten)) in a macro
Posted by Peter Maydell 5 years, 7 months ago
On 26 September 2018 at 16:59, Thomas Huth <thuth@redhat.com> wrote:

> +/*
> + * Clang 3.4 claims to be compatible with GCC 4.2, but does not have the
> + * "flatten" attribute, so we've got to handle Clang via __has_attribute here
> + */
> +#if defined(__clang__) && defined(__has_attribute)
> +# if __has_attribute(flatten)
> +#  define QEMU_FLATTEN __attribute__((flatten))
> +# endif
> +#elif !defined(__clang__) && QEMU_GNUC_PREREQ(4, 1)
> +# define QEMU_FLATTEN __attribute__((flatten))
> +#endif
> +#ifndef QEMU_FLATTEN
> +# define QEMU_FLATTEN
> +#endif

I think it would be cleaner to say: if we have __has_attribute,
trust it; otherwise fall back to version testing. Also, we
already require at least GCC 4.1 so a version test against
that is unnecessary. So something like:


#ifndef __has_attribute
#define __has_attribute(x) 0 / compatibility with older GCC */
#endif

/*
 * gcc doesn't provide __has_attribute() until gcc 5,
 * but we know all the gcc versions we support have flatten.
 * clang may not have flatten but always has __has_attribute().
 */
#if __has_attribute(flatten) || !defined(__clang__)
# define QEMU_FLATTEN __attribute__((flatten))
#else
# define QEMU_FLATTEN
#endif

> +
>  #ifndef __has_feature
>  #define __has_feature(x) 0 /* compatibility with non-clang compilers */
>  #endif
> --
> 1.8.3.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH] qemu/compiler: Wrap __attribute__((flatten)) in a macro
Posted by Thomas Huth 5 years, 7 months ago
On 2018-09-26 20:57, Peter Maydell wrote:
> On 26 September 2018 at 16:59, Thomas Huth <thuth@redhat.com> wrote:
> 
>> +/*
>> + * Clang 3.4 claims to be compatible with GCC 4.2, but does not have the
>> + * "flatten" attribute, so we've got to handle Clang via __has_attribute here
>> + */
>> +#if defined(__clang__) && defined(__has_attribute)
>> +# if __has_attribute(flatten)
>> +#  define QEMU_FLATTEN __attribute__((flatten))
>> +# endif
>> +#elif !defined(__clang__) && QEMU_GNUC_PREREQ(4, 1)
>> +# define QEMU_FLATTEN __attribute__((flatten))
>> +#endif
>> +#ifndef QEMU_FLATTEN
>> +# define QEMU_FLATTEN
>> +#endif
> 
> I think it would be cleaner to say: if we have __has_attribute,
> trust it; otherwise fall back to version testing. Also, we
> already require at least GCC 4.1 so a version test against
> that is unnecessary. So something like:
> 
> #ifndef __has_attribute
> #define __has_attribute(x) 0 / compatibility with older GCC */
> #endif
> 
> /*
>  * gcc doesn't provide __has_attribute() until gcc 5,
>  * but we know all the gcc versions we support have flatten.
>  * clang may not have flatten but always has __has_attribute().
>  */
> #if __has_attribute(flatten) || !defined(__clang__)
> # define QEMU_FLATTEN __attribute__((flatten))
> #else
> # define QEMU_FLATTEN
> #endif

Good idea, I'll send a v2 with that.

Speaking of the minimum GCC version that we require: Did we ever
officially define that? Or is it just a result from appendix C in our
qemu-doc? I guess the minimum GCC could be defined to 4.2 since that's
the compiler that was still used in OpenBSD recently, before they
switched to Clang?

Maybe we should add a list in our qemu-doc with some minimum versions
that we require (e.g. also glib), so that it is easier to look this up?

Also when searching for GNUC_PREREQ in the sources, there are at least
some occurances in tests/tcg/arm/fcvt.c and
include/fpu/softfloat-macros.h which we could simplify these days...

 Thomas

Re: [Qemu-devel] [PATCH] qemu/compiler: Wrap __attribute__((flatten)) in a macro
Posted by Peter Maydell 5 years, 7 months ago
On 27 September 2018 at 06:28, Thomas Huth <thuth@redhat.com> wrote:
> Speaking of the minimum GCC version that we require: Did we ever
> officially define that? Or is it just a result from appendix C in our
> qemu-doc? I guess the minimum GCC could be defined to 4.2 since that's
> the compiler that was still used in OpenBSD recently, before they
> switched to Clang?

It's currently 4.1 because we need the atomics support, and
nobody's had a strong reason to require a newer version than that.

> Maybe we should add a list in our qemu-doc with some minimum versions
> that we require (e.g. also glib), so that it is easier to look this up?

Yes. Also for gcc we could put in a "error out if not at least minver"
in compiler.h so I don't have to keep looking the minver up by
going through the git history :-)

> Also when searching for GNUC_PREREQ in the sources, there are at least
> some occurances in tests/tcg/arm/fcvt.c and
> include/fpu/softfloat-macros.h which we could simplify these days...

IIRC I updated all the QEMU_GNUC_PREREQ ones in commit fa54abb8c298f8;
I missed the fpu ones because they're using a different macro name,
and the fcvt.c is (a) a new file and (b) compiled with a compiler
for the guest which isn't necessarily the same as the one we used
to build QEMU itself.

thanks
-- PMM