[Qemu-devel] [PATCH 1/5] atomic: strip "const" from variables declared with typeof

[Qemu-devel] [PATCH 1/5] atomic: strip "const" from variables declared with typeof

[Pranith Kumar]

From: Paolo Bonzini <pbonzini@redhat.com>

With the latest clang, we have the following warning:

    /home/pranith/devops/code/qemu/include/qemu/seqlock.h:62:21: warning: passing 'typeof (*&sl->sequence) *' (aka 'const unsigned int *') to parameter of type 'unsigned int *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
        return unlikely(atomic_read(&sl->sequence) != start);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~
    /home/pranith/devops/code/qemu/include/qemu/atomic.h:58:25: note: expanded from macro 'atomic_read'
        __atomic_load(ptr, &_val, __ATOMIC_RELAXED);     \
                           ^~~~~

Stripping const is a bit tricky due to promotions, but it is doable
with either C11 _Generic or GCC extensions.  Use the latter.

Reported-by: Pranith Kumar <bobby.prani@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[pranith: Add conversion for bool type]
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 include/qemu/atomic.h | 54 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 7e13fca..43b0645 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -18,6 +18,48 @@
 /* Compiler barrier */
 #define barrier()   ({ asm volatile("" ::: "memory"); (void)0; })
 
+/* The variable that receives the old value of an atomically-accessed
+ * variable must be non-qualified, because atomic builtins return values
+ * through a pointer-type argument as in __atomic_load(&var, &old, MODEL).
+ *
+ * This macro has to handle types smaller than int manually, because of
+ * implicit promotion.  int and larger types, as well as pointers, can be
+ * converted to a non-qualified type just by applying a binary operator.
+ */
+#define typeof_strip_qual(expr)                                                    \
+  typeof(                                                                          \
+    __builtin_choose_expr(                                                         \
+      __builtin_types_compatible_p(typeof(expr), bool) ||                          \
+        __builtin_types_compatible_p(typeof(expr), const bool) ||                  \
+        __builtin_types_compatible_p(typeof(expr), volatile bool) ||               \
+        __builtin_types_compatible_p(typeof(expr), const volatile bool),           \
+        (bool)1,                                                                   \
+    __builtin_choose_expr(                                                         \
+      __builtin_types_compatible_p(typeof(expr), signed char) ||                   \
+        __builtin_types_compatible_p(typeof(expr), const signed char) ||           \
+        __builtin_types_compatible_p(typeof(expr), volatile signed char) ||        \
+        __builtin_types_compatible_p(typeof(expr), const volatile signed char),    \
+        (signed char)1,                                                            \
+    __builtin_choose_expr(                                                         \
+      __builtin_types_compatible_p(typeof(expr), unsigned char) ||                 \
+        __builtin_types_compatible_p(typeof(expr), const unsigned char) ||         \
+        __builtin_types_compatible_p(typeof(expr), volatile unsigned char) ||      \
+        __builtin_types_compatible_p(typeof(expr), const volatile unsigned char),  \
+        (unsigned char)1,                                                          \
+    __builtin_choose_expr(                                                         \
+      __builtin_types_compatible_p(typeof(expr), signed short) ||                  \
+        __builtin_types_compatible_p(typeof(expr), const signed short) ||          \
+        __builtin_types_compatible_p(typeof(expr), volatile signed short) ||       \
+        __builtin_types_compatible_p(typeof(expr), const volatile signed short),   \
+        (signed short)1,                                                           \
+    __builtin_choose_expr(                                                         \
+      __builtin_types_compatible_p(typeof(expr), unsigned short) ||                \
+        __builtin_types_compatible_p(typeof(expr), const unsigned short) ||        \
+        __builtin_types_compatible_p(typeof(expr), volatile unsigned short) ||     \
+        __builtin_types_compatible_p(typeof(expr), const volatile unsigned short), \
+        (unsigned short)1,                                                         \
+      (expr)+0))))))
+
 #ifdef __ATOMIC_RELAXED
 /* For C11 atomic ops */
 
@@ -54,7 +96,7 @@
 #define atomic_read(ptr)                              \
     ({                                                \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
-    typeof(*ptr) _val;                                \
+    typeof_strip_qual(*ptr) _val;                     \
      __atomic_load(ptr, &_val, __ATOMIC_RELAXED);     \
     _val;                                             \
     })
@@ -80,7 +122,7 @@
 #define atomic_rcu_read(ptr)                          \
     ({                                                \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
-    typeof(*ptr) _val;                                \
+    typeof_strip_qual(*ptr) _val;                     \
     atomic_rcu_read__nocheck(ptr, &_val);             \
     _val;                                             \
     })
@@ -103,7 +145,7 @@
 #define atomic_mb_read(ptr)                             \
     ({                                                  \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
-    typeof(*ptr) _val;                                  \
+    typeof_strip_qual(*ptr) _val;                       \
      __atomic_load(ptr, &_val, __ATOMIC_RELAXED);       \
      smp_rmb();                                         \
     _val;                                               \
@@ -120,7 +162,7 @@
 #define atomic_mb_read(ptr)                             \
     ({                                                  \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
-    typeof(*ptr) _val;                                  \
+    typeof_strip_qual(*ptr) _val;                       \
     __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST);        \
     _val;                                               \
     })
@@ -137,7 +179,7 @@
 
 #define atomic_xchg(ptr, i)    ({                           \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));       \
-    typeof(*ptr) _new = (i), _old;                          \
+    typeof_strip_qual(*ptr) _new = (i), _old;               \
     __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \
     _old;                                                   \
 })
@@ -146,7 +188,7 @@
 #define atomic_cmpxchg(ptr, old, new)                                   \
     ({                                                                  \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));                   \
-    typeof(*ptr) _old = (old), _new = (new);                            \
+    typeof_strip_qual(*ptr) _old = (old), _new = (new);                 \
     __atomic_compare_exchange(ptr, &_old, &_new, false,                 \
                               __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);      \
     _old;                                                               \
-- 
2.9.2

[Qemu-devel] [PATCH 2/5] softfloat: Fix warn about implicit conversion from int to int8_t

[Pranith Kumar]

Change the flag type to 'int' to fix the implicit conversion error.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 fpu/softfloat-specialize.h | 2 +-
 include/fpu/softfloat.h    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index 43d0890..46b4091 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -197,7 +197,7 @@ float128 float128_default_nan(float_status *status)
 | should be simply `float_exception_flags |= flags;'.
 *----------------------------------------------------------------------------*/
 
-void float_raise(int8_t flags, float_status *status)
+void float_raise(int flags, float_status *status)
 {
     status->float_exception_flags |= flags;
 }
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 0e57ee5..416cf7a 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -196,9 +196,9 @@ enum {
 };
 
 typedef struct float_status {
+    int         float_exception_flags;
     signed char float_detect_tininess;
     signed char float_rounding_mode;
-    signed char float_exception_flags;
     signed char floatx80_rounding_precision;
     /* should denormalised results go to zero and set the inexact flag? */
     flag flush_to_zero;
@@ -274,7 +274,7 @@ static inline flag get_default_nan_mode(float_status *status)
 | Routine to raise any or all of the software IEC/IEEE floating-point
 | exception flags.
 *----------------------------------------------------------------------------*/
-void float_raise(int8_t flags, float_status *status);
+void float_raise(int flags, float_status *status);
 
 /*----------------------------------------------------------------------------
 | If `a' is denormal and we are in flush-to-zero mode then set the
-- 
2.9.2

Re: [Qemu-devel] [PATCH 2/5] softfloat: Fix warn about implicit conversion from int to int8_t

[Aurelien Jarno]

On 2016-08-09 15:02, Pranith Kumar wrote:
> Change the flag type to 'int' to fix the implicit conversion error.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  fpu/softfloat-specialize.h | 2 +-
>  include/fpu/softfloat.h    | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index 43d0890..46b4091 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -197,7 +197,7 @@ float128 float128_default_nan(float_status *status)
>  | should be simply `float_exception_flags |= flags;'.
>  *----------------------------------------------------------------------------*/
>  
> -void float_raise(int8_t flags, float_status *status)
> +void float_raise(int flags, float_status *status)
>  {
>      status->float_exception_flags |= flags;
>  }
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index 0e57ee5..416cf7a 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -196,9 +196,9 @@ enum {
>  };
>  
>  typedef struct float_status {
> +    int         float_exception_flags;
>      signed char float_detect_tininess;
>      signed char float_rounding_mode;
> -    signed char float_exception_flags;
>      signed char floatx80_rounding_precision;
>      /* should denormalised results go to zero and set the inexact flag? */
>      flag flush_to_zero;

This changes the size of the structure, and thus of the CPU*State
structures. I don't think it's something we want to do, especially given
we currently only use 7 flags, so 7 bits and that fits in a char.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/5] softfloat: Fix warn about implicit conversion from int to int8_t

[Peter Maydell]

On 9 August 2016 at 20:16, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On 2016-08-09 15:02, Pranith Kumar wrote:
>> Change the flag type to 'int' to fix the implicit conversion error.
>>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
>> ---
>>  fpu/softfloat-specialize.h | 2 +-
>>  include/fpu/softfloat.h    | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
>> index 43d0890..46b4091 100644
>> --- a/fpu/softfloat-specialize.h
>> +++ b/fpu/softfloat-specialize.h
>> @@ -197,7 +197,7 @@ float128 float128_default_nan(float_status *status)
>>  | should be simply `float_exception_flags |= flags;'.
>>  *----------------------------------------------------------------------------*/
>>
>> -void float_raise(int8_t flags, float_status *status)
>> +void float_raise(int flags, float_status *status)
>>  {
>>      status->float_exception_flags |= flags;
>>  }
>> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
>> index 0e57ee5..416cf7a 100644
>> --- a/include/fpu/softfloat.h
>> +++ b/include/fpu/softfloat.h
>> @@ -196,9 +196,9 @@ enum {
>>  };
>>
>>  typedef struct float_status {
>> +    int         float_exception_flags;
>>      signed char float_detect_tininess;
>>      signed char float_rounding_mode;
>> -    signed char float_exception_flags;
>>      signed char floatx80_rounding_precision;
>>      /* should denormalised results go to zero and set the inexact flag? */
>>      flag flush_to_zero;
>
> This changes the size of the structure, and thus of the CPU*State
> structures. I don't think it's something we want to do, especially given
> we currently only use 7 flags, so 7 bits and that fits in a char.

It does, but only by four bytes, which I didn't think was that
big a deal. If we want to keep it to one byte then I think
making it a uint8_t is probably better than leaving it as
signed char, given we're definitely not treating it as a
signed value.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] softfloat: Fix warn about implicit conversion from int to int8_t

[Aurelien Jarno]

On 2016-08-09 22:12, Peter Maydell wrote:
> On 9 August 2016 at 20:16, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On 2016-08-09 15:02, Pranith Kumar wrote:
> >> Change the flag type to 'int' to fix the implicit conversion error.
> >>
> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> >> ---
> >>  fpu/softfloat-specialize.h | 2 +-
> >>  include/fpu/softfloat.h    | 4 ++--
> >>  2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> >> index 43d0890..46b4091 100644
> >> --- a/fpu/softfloat-specialize.h
> >> +++ b/fpu/softfloat-specialize.h
> >> @@ -197,7 +197,7 @@ float128 float128_default_nan(float_status *status)
> >>  | should be simply `float_exception_flags |= flags;'.
> >>  *----------------------------------------------------------------------------*/
> >>
> >> -void float_raise(int8_t flags, float_status *status)
> >> +void float_raise(int flags, float_status *status)
> >>  {
> >>      status->float_exception_flags |= flags;
> >>  }
> >> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> >> index 0e57ee5..416cf7a 100644
> >> --- a/include/fpu/softfloat.h
> >> +++ b/include/fpu/softfloat.h
> >> @@ -196,9 +196,9 @@ enum {
> >>  };
> >>
> >>  typedef struct float_status {
> >> +    int         float_exception_flags;
> >>      signed char float_detect_tininess;
> >>      signed char float_rounding_mode;
> >> -    signed char float_exception_flags;
> >>      signed char floatx80_rounding_precision;
> >>      /* should denormalised results go to zero and set the inexact flag? */
> >>      flag flush_to_zero;
> >
> > This changes the size of the structure, and thus of the CPU*State
> > structures. I don't think it's something we want to do, especially given
> > we currently only use 7 flags, so 7 bits and that fits in a char.
> 
> It does, but only by four bytes, which I didn't think was that
> big a deal. If we want to keep it to one byte then I think

Indeed it's not a lot, but if we do that with everything that goes into
the CPU*state structures, it has a significant impact.

> making it a uint8_t is probably better than leaving it as
> signed char, given we're definitely not treating it as a
> signed value.

I agree the signed char type is very strange here, it is probably there
for historical reasons. I guess using a uint8_t would indeed be the
correct short term fix.

The long term fix is probably to change this whole structure as a
bitfield of unsigned int, like we already do in the tcg code.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/5] softfloat: Fix warn about implicit conversion from int to int8_t

[Richard Henderson]

On 08/10/2016 12:32 AM, Pranith Kumar wrote:
>  typedef struct float_status {
> +    int         float_exception_flags;
>      signed char float_detect_tininess;
>      signed char float_rounding_mode;
> -    signed char float_exception_flags;

Given that there are no flags outside 8 bits, why is this supposed to help?


r~

Re: [Qemu-devel] [PATCH 2/5] softfloat: Fix warn about implicit conversion from int to int8_t

[Peter Maydell]

On 10 August 2016 at 11:32, Richard Henderson <rth@twiddle.net> wrote:
> On 08/10/2016 12:32 AM, Pranith Kumar wrote:
>>
>>  typedef struct float_status {
>> +    int         float_exception_flags;
>>      signed char float_detect_tininess;
>>      signed char float_rounding_mode;
>> -    signed char float_exception_flags;
>
>
> Given that there are no flags outside 8 bits, why is this supposed to help?

It's just consistency -- using the same type all the way through
from the set_float_exception_flags() prototype to the field
in the structure. When we were discussing this on IRC that
seemed a reasonable approach to me.

What clang is specifically warning about is if you pass
float_flag_output_denormal to set_float_exception_flags():
that's a positive number (128), which gets implicitly
converted to a negative number when it's assigned to
the signed char. Using a type wide enough to store
128 all the way through fixes this. (Unsigned char would
work too.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] softfloat: Fix warn about implicit conversion from int to int8_t

[Paolo Bonzini]

On 10/08/2016 12:37, Peter Maydell wrote:
> On 10 August 2016 at 11:32, Richard Henderson <rth@twiddle.net> wrote:
>> On 08/10/2016 12:32 AM, Pranith Kumar wrote:
>>>
>>>  typedef struct float_status {
>>> +    int         float_exception_flags;
>>>      signed char float_detect_tininess;
>>>      signed char float_rounding_mode;
>>> -    signed char float_exception_flags;
>>
>>
>> Given that there are no flags outside 8 bits, why is this supposed to help?
> 
> It's just consistency -- using the same type all the way through
> from the set_float_exception_flags() prototype to the field
> in the structure. When we were discussing this on IRC that
> seemed a reasonable approach to me.
> 
> What clang is specifically warning about is if you pass
> float_flag_output_denormal to set_float_exception_flags():
> that's a positive number (128), which gets implicitly
> converted to a negative number when it's assigned to
> the signed char. Using a type wide enough to store
> 128 all the way through fixes this. (Unsigned char would
> work too.)

That seems better.  By the way this use of "signed char" is a real bug;
get_float_exception_flags is returning a sign-extended value, which is
turned into a positive value here:

  if(nRc == 1 && get_float_exception_flags(&fpa11->fp_status))
  {
    //printf("fef 0x%x\n",float_exception_flags);
    nRc = -get_float_exception_flags(&fpa11->fp_status);
  }

  //printf("returning %d\n",nRc);
  return(nRc);

Thanks,

Paolo

[Qemu-devel] [PATCH 3/5] Disable warn about left shifts of negative values

[Pranith Kumar]

It seems like there's no good reason for the compiler to exploit the
undefinedness of left shifts.  GCC explicitly documents that they do not
use at all this possibility and, while they also say this is subject
to change, they have been saying this for 10 years (since the wording
appeared in the GCC 4.0 manual).

Disable these warnings by passing in -Wno-shift-negative-value.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[pranith: forward-port part of patch to 2.7]
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 HACKING   | 4 ++++
 configure | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/HACKING b/HACKING
index 058aa8f..20a9101 100644
--- a/HACKING
+++ b/HACKING
@@ -158,6 +158,10 @@ painful. These are:
  * you may assume that right shift of a signed integer duplicates
    the sign bit (ie it is an arithmetic shift, not a logical shift)
 
+In addition, QEMU assumes that the compiler does not use the latitude
+given in C99 and C11 to treat aspects of signed '<<' as undefined, as
+documented in the GNU Compiler Collection manual starting at version 4.0.
+
 7. Error handling and reporting
 
 7.1 Reporting errors to the human user
diff --git a/configure b/configure
index f57fcc6..8d84919 100755
--- a/configure
+++ b/configure
@@ -1452,7 +1452,7 @@ fi
 gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
 gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
 gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
-gcc_flags="-Wendif-labels $gcc_flags"
+gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
 gcc_flags="-Wno-initializer-overrides $gcc_flags"
 gcc_flags="-Wno-string-plus-int $gcc_flags"
 # Note that we do not add -Werror to gcc_flags here, because that would
-- 
2.9.2

[Qemu-devel] [PATCH 4/5] clang: Fix warning reg. expansion to 'defined'

[Pranith Kumar]

Clang produces the following warning. The warning is detailed here:
https://reviews.llvm.org/D15866. Fix the warning.

/home/pranith/devops/code/qemu/hw/display/qxl.c:507:5: warning: macro expansion producing 'defined' has undefined behavior [-Wexpansion-to-defined]
    ^
/home/pranith/devops/code/qemu/include/ui/qemu-spice.h:46:5: note: expanded from macro 'SPICE_NEEDS_SET_MM_TIME'
  (!defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06))
    ^
/home/pranith/devops/code/qemu/hw/display/qxl.c:1074:5: warning: macro expansion producing 'defined' has undefined behavior [-Wexpansion-to-defined]
    ^
/home/pranith/devops/code/qemu/include/ui/qemu-spice.h:46:5: note: expanded from macro 'SPICE_NEEDS_SET_MM_TIME'
  (!defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06))

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 include/ui/qemu-spice.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index edad5e7..75e1239 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -42,8 +42,11 @@ int qemu_spice_set_pw_expire(time_t expires);
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
                             const char *subject);
 
-#define SPICE_NEEDS_SET_MM_TIME                                       \
-  (!defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06))
+#if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
+#define SPICE_NEEDS_SET_MM_TIME 1
+#else
+#define SPICE_NEEDS_SET_MM_TIME 0
+#endif
 
 #if SPICE_SERVER_VERSION >= 0x000c02
 void qemu_spice_register_ports(void);
-- 
2.9.2

[Qemu-devel] [PATCH 5/5] target-arm: Fix warn about implicit conversion

[Pranith Kumar]

Clang warns about an implicit conversion as follows:

/mnt/devops/code/qemu/target-arm/neon_helper.c:1075:1: warning: implicit conversion from 'int' to 'int8_t' (aka 'signed char') changes value from 128 to -128 [-Wconstant-conversion]
NEON_VOP_ENV(qrshl_s8, neon_s8, 4)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mnt/devops/code/qemu/target-arm/neon_helper.c:116:83: note: expanded from macro 'NEON_VOP_ENV'
uint32_t HELPER(glue(neon_,name))(CPUARMState *env, uint32_t arg1, uint32_t arg2) \
                                                                                  ^
/mnt/devops/code/qemu/target-arm/neon_helper.c:106:5: note: expanded from macro '\
NEON_VOP_BODY'
    NEON_DO##n; \
    ^~~~~~~~~~
<scratch space>:21:1: note: expanded from here
NEON_DO4
^~~~~~~~
/mnt/devops/code/qemu/target-arm/neon_helper.c:93:5: note: expanded from macro 'NEON_DO4'
    NEON_FN(vdest.v1, vsrc1.v1, vsrc2.v1); \
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mnt/devops/code/qemu/target-arm/neon_helper.c:1054:23: note: expanded from macro 'NEON_FN'
            dest = (1 << (sizeof(src1) * 8 - 1)); \
                 ~  ~~^~~~~~~~~~~~~~~~~~~~~~~~~

Fix it by casting to appropriate type.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 target-arm/neon_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 1f1844f..ebdf7c9 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -1051,7 +1051,7 @@ uint64_t HELPER(neon_qrshl_u64)(CPUARMState *env, uint64_t val, uint64_t shiftop
     if (tmp >= (ssize_t)sizeof(src1) * 8) { \
         if (src1) { \
             SET_QC(); \
-            dest = (1 << (sizeof(src1) * 8 - 1)); \
+            dest = (typeof(dest))(1 << (sizeof(src1) * 8 - 1)); \
             if (src1 > 0) { \
                 dest--; \
             } \
-- 
2.9.2

Re: [Qemu-devel] [PATCH 5/5] target-arm: Fix warn about implicit conversion

[Peter Maydell]

On 9 August 2016 at 20:02, Pranith Kumar <bobby.prani@gmail.com> wrote:
> Clang warns about an implicit conversion as follows:
>
> /mnt/devops/code/qemu/target-arm/neon_helper.c:1075:1: warning: implicit conversion from 'int' to 'int8_t' (aka 'signed char') changes value from 128 to -128 [-Wconstant-conversion]
> NEON_VOP_ENV(qrshl_s8, neon_s8, 4)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /mnt/devops/code/qemu/target-arm/neon_helper.c:116:83: note: expanded from macro 'NEON_VOP_ENV'
> uint32_t HELPER(glue(neon_,name))(CPUARMState *env, uint32_t arg1, uint32_t arg2) \
>                                                                                   ^
> /mnt/devops/code/qemu/target-arm/neon_helper.c:106:5: note: expanded from macro '\
> NEON_VOP_BODY'
>     NEON_DO##n; \
>     ^~~~~~~~~~
> <scratch space>:21:1: note: expanded from here
> NEON_DO4
> ^~~~~~~~
> /mnt/devops/code/qemu/target-arm/neon_helper.c:93:5: note: expanded from macro 'NEON_DO4'
>     NEON_FN(vdest.v1, vsrc1.v1, vsrc2.v1); \
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /mnt/devops/code/qemu/target-arm/neon_helper.c:1054:23: note: expanded from macro 'NEON_FN'
>             dest = (1 << (sizeof(src1) * 8 - 1)); \
>                  ~  ~~^~~~~~~~~~~~~~~~~~~~~~~~~
>
> Fix it by casting to appropriate type.
>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  target-arm/neon_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
> index 1f1844f..ebdf7c9 100644
> --- a/target-arm/neon_helper.c
> +++ b/target-arm/neon_helper.c
> @@ -1051,7 +1051,7 @@ uint64_t HELPER(neon_qrshl_u64)(CPUARMState *env, uint64_t val, uint64_t shiftop
>      if (tmp >= (ssize_t)sizeof(src1) * 8) { \
>          if (src1) { \
>              SET_QC(); \
> -            dest = (1 << (sizeof(src1) * 8 - 1)); \
> +            dest = (typeof(dest))(1 << (sizeof(src1) * 8 - 1)); \
>              if (src1 > 0) { \
>                  dest--; \
>              } \

A bit ugly but I guess it works. (The code is
deliberately setting dest to "most negative integer
that fits into src1 type".)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/5] target-arm: Fix warn about implicit conversion

[Peter Maydell]

On 11 August 2016 at 11:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 August 2016 at 20:02, Pranith Kumar <bobby.prani@gmail.com> wrote:
>> Clang warns about an implicit conversion as follows:
>>
>> /mnt/devops/code/qemu/target-arm/neon_helper.c:1075:1: warning: implicit conversion from 'int' to 'int8_t' (aka 'signed char') changes value from 128 to -128 [-Wconstant-conversion]
>> NEON_VOP_ENV(qrshl_s8, neon_s8, 4)
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /mnt/devops/code/qemu/target-arm/neon_helper.c:116:83: note: expanded from macro 'NEON_VOP_ENV'
>> uint32_t HELPER(glue(neon_,name))(CPUARMState *env, uint32_t arg1, uint32_t arg2) \
>>                                                                                   ^
>> /mnt/devops/code/qemu/target-arm/neon_helper.c:106:5: note: expanded from macro '\
>> NEON_VOP_BODY'
>>     NEON_DO##n; \
>>     ^~~~~~~~~~
>> <scratch space>:21:1: note: expanded from here
>> NEON_DO4
>> ^~~~~~~~
>> /mnt/devops/code/qemu/target-arm/neon_helper.c:93:5: note: expanded from macro 'NEON_DO4'
>>     NEON_FN(vdest.v1, vsrc1.v1, vsrc2.v1); \
>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /mnt/devops/code/qemu/target-arm/neon_helper.c:1054:23: note: expanded from macro 'NEON_FN'
>>             dest = (1 << (sizeof(src1) * 8 - 1)); \
>>                  ~  ~~^~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Fix it by casting to appropriate type.
>>
>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
>> ---
>>  target-arm/neon_helper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
>> index 1f1844f..ebdf7c9 100644
>> --- a/target-arm/neon_helper.c
>> +++ b/target-arm/neon_helper.c
>> @@ -1051,7 +1051,7 @@ uint64_t HELPER(neon_qrshl_u64)(CPUARMState *env, uint64_t val, uint64_t shiftop
>>      if (tmp >= (ssize_t)sizeof(src1) * 8) { \
>>          if (src1) { \
>>              SET_QC(); \
>> -            dest = (1 << (sizeof(src1) * 8 - 1)); \
>> +            dest = (typeof(dest))(1 << (sizeof(src1) * 8 - 1)); \
>>              if (src1 > 0) { \
>>                  dest--; \
>>              } \
>
> A bit ugly but I guess it works. (The code is
> deliberately setting dest to "most negative integer
> that fits into src1 type".)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Applied to master, thanks.

-- PMM

Re: [Qemu-devel] [PATCH 1/5] atomic: strip "const" from variables declared with typeof

[Paolo Bonzini]

On 09/08/2016 21:02, Pranith Kumar wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> With the latest clang, we have the following warning:
> 
>     /home/pranith/devops/code/qemu/include/qemu/seqlock.h:62:21: warning: passing 'typeof (*&sl->sequence) *' (aka 'const unsigned int *') to parameter of type 'unsigned int *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
>         return unlikely(atomic_read(&sl->sequence) != start);
>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~
>     /home/pranith/devops/code/qemu/include/qemu/atomic.h:58:25: note: expanded from macro 'atomic_read'
>         __atomic_load(ptr, &_val, __ATOMIC_RELAXED);     \
>                            ^~~~~
> 
> Stripping const is a bit tricky due to promotions, but it is doable
> with either C11 _Generic or GCC extensions.  Use the latter.
> 
> Reported-by: Pranith Kumar <bobby.prani@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [pranith: Add conversion for bool type]
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  include/qemu/atomic.h | 54 +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
> index 7e13fca..43b0645 100644
> --- a/include/qemu/atomic.h
> +++ b/include/qemu/atomic.h
> @@ -18,6 +18,48 @@
>  /* Compiler barrier */
>  #define barrier()   ({ asm volatile("" ::: "memory"); (void)0; })
>  
> +/* The variable that receives the old value of an atomically-accessed
> + * variable must be non-qualified, because atomic builtins return values
> + * through a pointer-type argument as in __atomic_load(&var, &old, MODEL).
> + *
> + * This macro has to handle types smaller than int manually, because of
> + * implicit promotion.  int and larger types, as well as pointers, can be
> + * converted to a non-qualified type just by applying a binary operator.
> + */
> +#define typeof_strip_qual(expr)                                                    \
> +  typeof(                                                                          \
> +    __builtin_choose_expr(                                                         \
> +      __builtin_types_compatible_p(typeof(expr), bool) ||                          \
> +        __builtin_types_compatible_p(typeof(expr), const bool) ||                  \
> +        __builtin_types_compatible_p(typeof(expr), volatile bool) ||               \
> +        __builtin_types_compatible_p(typeof(expr), const volatile bool),           \
> +        (bool)1,                                                                   \
> +    __builtin_choose_expr(                                                         \
> +      __builtin_types_compatible_p(typeof(expr), signed char) ||                   \
> +        __builtin_types_compatible_p(typeof(expr), const signed char) ||           \
> +        __builtin_types_compatible_p(typeof(expr), volatile signed char) ||        \
> +        __builtin_types_compatible_p(typeof(expr), const volatile signed char),    \
> +        (signed char)1,                                                            \
> +    __builtin_choose_expr(                                                         \
> +      __builtin_types_compatible_p(typeof(expr), unsigned char) ||                 \
> +        __builtin_types_compatible_p(typeof(expr), const unsigned char) ||         \
> +        __builtin_types_compatible_p(typeof(expr), volatile unsigned char) ||      \
> +        __builtin_types_compatible_p(typeof(expr), const volatile unsigned char),  \
> +        (unsigned char)1,                                                          \
> +    __builtin_choose_expr(                                                         \
> +      __builtin_types_compatible_p(typeof(expr), signed short) ||                  \
> +        __builtin_types_compatible_p(typeof(expr), const signed short) ||          \
> +        __builtin_types_compatible_p(typeof(expr), volatile signed short) ||       \
> +        __builtin_types_compatible_p(typeof(expr), const volatile signed short),   \
> +        (signed short)1,                                                           \
> +    __builtin_choose_expr(                                                         \
> +      __builtin_types_compatible_p(typeof(expr), unsigned short) ||                \
> +        __builtin_types_compatible_p(typeof(expr), const unsigned short) ||        \
> +        __builtin_types_compatible_p(typeof(expr), volatile unsigned short) ||     \
> +        __builtin_types_compatible_p(typeof(expr), const volatile unsigned short), \
> +        (unsigned short)1,                                                         \
> +      (expr)+0))))))
> +
>  #ifdef __ATOMIC_RELAXED
>  /* For C11 atomic ops */
>  
> @@ -54,7 +96,7 @@
>  #define atomic_read(ptr)                              \
>      ({                                                \
>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
> -    typeof(*ptr) _val;                                \
> +    typeof_strip_qual(*ptr) _val;                     \
>       __atomic_load(ptr, &_val, __ATOMIC_RELAXED);     \
>      _val;                                             \
>      })
> @@ -80,7 +122,7 @@
>  #define atomic_rcu_read(ptr)                          \
>      ({                                                \
>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
> -    typeof(*ptr) _val;                                \
> +    typeof_strip_qual(*ptr) _val;                     \
>      atomic_rcu_read__nocheck(ptr, &_val);             \
>      _val;                                             \
>      })
> @@ -103,7 +145,7 @@
>  #define atomic_mb_read(ptr)                             \
>      ({                                                  \
>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
> -    typeof(*ptr) _val;                                  \
> +    typeof_strip_qual(*ptr) _val;                       \
>       __atomic_load(ptr, &_val, __ATOMIC_RELAXED);       \
>       smp_rmb();                                         \
>      _val;                                               \
> @@ -120,7 +162,7 @@
>  #define atomic_mb_read(ptr)                             \
>      ({                                                  \
>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
> -    typeof(*ptr) _val;                                  \
> +    typeof_strip_qual(*ptr) _val;                       \
>      __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST);        \
>      _val;                                               \
>      })
> @@ -137,7 +179,7 @@
>  
>  #define atomic_xchg(ptr, i)    ({                           \
>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));       \
> -    typeof(*ptr) _new = (i), _old;                          \
> +    typeof_strip_qual(*ptr) _new = (i), _old;               \
>      __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \
>      _old;                                                   \
>  })
> @@ -146,7 +188,7 @@
>  #define atomic_cmpxchg(ptr, old, new)                                   \
>      ({                                                                  \
>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));                   \
> -    typeof(*ptr) _old = (old), _new = (new);                            \
> +    typeof_strip_qual(*ptr) _old = (old), _new = (new);                 \
>      __atomic_compare_exchange(ptr, &_old, &_new, false,                 \
>                                __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);      \
>      _old;                                                               \
> 

Queued patches 1, 3, 4.

Paolo