[Qemu-devel] [PATCH 0/2] target-arm: Avoid clang sanitizer warnings

[Qemu-devel] [PATCH 0/2] target-arm: Avoid clang sanitizer warnings

[Peter Maydell]

These patches avoid some clang sanitizer warnings triggered
on target-arm code which inadvertently shifts into the sign
bit of a signed integer (which is undefined behaviour in C).

(For more info on the sanitizer see
http://blog.regehr.org/archives/963 ; the basic approach
is to install clang 3.3 and then configure QEMU with
 --cc=clang --extra-cflags='-fsanitize=undefined' ; the resulting
QEMU will print warnings at runtime for various kinds of
integer undefined behaviour.)

Peter Maydell (2):
  target-arm: Use sextract32() in branch decode
  target-arm: Avoid "1 << 31" undefined behaviour

 target-arm/cpu.h       |    2 +-
 target-arm/helper.c    |    4 ++--
 target-arm/translate.c |    5 +++--
 3 files changed, 6 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] target-arm: Use sextract32() in branch decode

[Peter Maydell]

In the decode of ARM B and BL insns, swap the order of the
"append 2 implicit zeros to imm24" and the sign extend, and
use the new sextract32() utility function to do the latter.
This avoids a direct dependency on the undefined C behaviour
of shifting into the sign bit of an integer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index d1e8538..ebf5d4f 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -28,6 +28,7 @@
 #include "disas/disas.h"
 #include "tcg-op.h"
 #include "qemu/log.h"
+#include "qemu/bitops.h"
 
 #include "helper.h"
 #define GEN_HELPER 1
@@ -7956,8 +7957,8 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                     tcg_gen_movi_i32(tmp, val);
                     store_reg(s, 14, tmp);
                 }
-                offset = (((int32_t)insn << 8) >> 8);
-                val += (offset << 2) + 4;
+                offset = sextract32(insn << 2, 0, 26);
+                val += offset + 4;
                 gen_jmp(s, val);
             }
             break;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/2] target-arm: Use sextract32() in branch decode

[Richard Henderson]

On 08/23/2013 09:12 AM, Peter Maydell wrote:
> -                offset = (((int32_t)insn << 8) >> 8);
> -                val += (offset << 2) + 4;
> +                offset = sextract32(insn << 2, 0, 26);
> +                val += offset + 4;

I read this incorrectly at first, considering the shift of insn, and
I wonder if it's really the best way to write this because of that.

What about just changing the one line to sextract(insn, 0, 24)?

The second line by itself ought not trigger a warning from clang,
because the << 2 never changes the sign bit.  If it still does,
perhaps just multiply by 4 instead...

It's a stupid warning.  When was the last ones-compliment machine built?


r~

Re: [Qemu-devel] [PATCH 1/2] target-arm: Use sextract32() in branch decode

[Peter Maydell]

On 23 August 2013 19:09, Richard Henderson <rth@twiddle.net> wrote:
> On 08/23/2013 09:12 AM, Peter Maydell wrote:
>> -                offset = (((int32_t)insn << 8) >> 8);
>> -                val += (offset << 2) + 4;
>> +                offset = sextract32(insn << 2, 0, 26);
>> +                val += offset + 4;
>
> I read this incorrectly at first, considering the shift of insn, and
> I wonder if it's really the best way to write this because of that.
>
> What about just changing the one line to sextract(insn, 0, 24)?
>
> The second line by itself ought not trigger a warning from clang,
> because the << 2 never changes the sign bit.  If it still does,
> perhaps just multiply by 4 instead...

No, left shift of a negative value is undefined: "If E1 has a signed type
and nonnegative value, and E1 × 2E2 is representable in the result
type, then that is the resulting value; otherwise, the behavior is
undefined."

Also the ARM ARM pseudocode defines this operation as "first
append two zero bits and then sign extend" so I prefer it if we
actually implement it that way round.

> It's a stupid warning.  When was the last ones-compliment machine built?

The stupidity is that the C standard hasn't mandated 2s-complement.

-- PMM

[Qemu-devel] [PATCH 2/2] target-arm: Avoid "1 << 31" undefined behaviour

[Peter Maydell]

Avoid the undefined behaviour of "1 << 31" by using 1U to make
the shift be of an unsigned value rather than shifting into the
sign bit of a signed integer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h    |    2 +-
 target-arm/helper.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index f2abdf3..f3bd501 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -285,7 +285,7 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, target_ulong address, int rw,
 #define CPSR_V (1 << 28)
 #define CPSR_C (1 << 29)
 #define CPSR_Z (1 << 30)
-#define CPSR_N (1 << 31)
+#define CPSR_N (1U << 31)
 #define CPSR_NZCV (CPSR_N | CPSR_Z | CPSR_C | CPSR_V)
 
 #define CPSR_IT (CPSR_IT_0_1 | CPSR_IT_2_7)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index f4e1b06..b13edf1 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -972,7 +972,7 @@ static int par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 static inline bool extended_addresses_enabled(CPUARMState *env)
 {
     return arm_feature(env, ARM_FEATURE_LPAE)
-        && (env->cp15.c2_control & (1 << 31));
+        && (env->cp15.c2_control & (1U << 31));
 }
 
 static int ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
@@ -1385,7 +1385,7 @@ static int mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
      * so these bits always RAZ.
      */
     if (arm_feature(env, ARM_FEATURE_V7MP)) {
-        mpidr |= (1 << 31);
+        mpidr |= (1U << 31);
         /* Cores which are uniprocessor (non-coherent)
          * but still implement the MP extensions set
          * bit 30. (For instance, A9UP.) However we do
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/2] target-arm: Avoid "1 << 31" undefined behaviour

[Richard Henderson]

On 08/23/2013 09:12 AM, Peter Maydell wrote:
>  #define CPSR_V (1 << 28)
>  #define CPSR_C (1 << 29)
>  #define CPSR_Z (1 << 30)
> -#define CPSR_N (1 << 31)
> +#define CPSR_N (1U << 31)
>  #define CPSR_NZCV (CPSR_N | CPSR_Z | CPSR_C | CPSR_V)

You'd be better off making all of the CPSR bits unsigned, I think.



r~

Re: [Qemu-devel] [PATCH 2/2] target-arm: Avoid "1 << 31" undefined behaviour

[Peter Maydell]

On 23 August 2013 19:11, Richard Henderson <rth@twiddle.net> wrote:
> On 08/23/2013 09:12 AM, Peter Maydell wrote:
>>  #define CPSR_V (1 << 28)
>>  #define CPSR_C (1 << 29)
>>  #define CPSR_Z (1 << 30)
>> -#define CPSR_N (1 << 31)
>> +#define CPSR_N (1U << 31)
>>  #define CPSR_NZCV (CPSR_N | CPSR_Z | CPSR_C | CPSR_V)
>
> You'd be better off making all of the CPSR bits unsigned, I think.

Agreed; let's be consistent.

-- PMM