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

[Qemu-devel] [PATCH v2 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).

Changes v1->v2:
 * made all the CPSR_* defines unsigned for consistency
   (suggested by rth in review)

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

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

-- 
1.7.9.5

[Qemu-devel] [PATCH v2 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 4f4a0a9..8bcfaf3 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
@@ -7957,8 +7958,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

[Qemu-devel] [PATCH v2 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. For consistency, we make all the
CPSR_* constants unsigned, though the only one which triggers
undefined behaviour is CPSR_N.

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

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index f2abdf3..af7cf8a 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -270,22 +270,22 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, target_ulong address, int rw,
                               int mmu_idx);
 #define cpu_handle_mmu_fault cpu_arm_handle_mmu_fault
 
-#define CPSR_M (0x1f)
-#define CPSR_T (1 << 5)
-#define CPSR_F (1 << 6)
-#define CPSR_I (1 << 7)
-#define CPSR_A (1 << 8)
-#define CPSR_E (1 << 9)
-#define CPSR_IT_2_7 (0xfc00)
-#define CPSR_GE (0xf << 16)
-#define CPSR_RESERVED (0xf << 20)
-#define CPSR_J (1 << 24)
-#define CPSR_IT_0_1 (3 << 25)
-#define CPSR_Q (1 << 27)
-#define CPSR_V (1 << 28)
-#define CPSR_C (1 << 29)
-#define CPSR_Z (1 << 30)
-#define CPSR_N (1 << 31)
+#define CPSR_M (0x1fU)
+#define CPSR_T (1U << 5)
+#define CPSR_F (1U << 6)
+#define CPSR_I (1U << 7)
+#define CPSR_A (1U << 8)
+#define CPSR_E (1U << 9)
+#define CPSR_IT_2_7 (0xfc00U)
+#define CPSR_GE (0xfU << 16)
+#define CPSR_RESERVED (0xfU << 20)
+#define CPSR_J (1U << 24)
+#define CPSR_IT_0_1 (3U << 25)
+#define CPSR_Q (1U << 27)
+#define CPSR_V (1U << 28)
+#define CPSR_C (1U << 29)
+#define CPSR_Z (1U << 30)
+#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 e51ef20..c1a68c7 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 v2 0/2] target-arm: Avoid clang sanitizer warnings

[Richard Henderson]

On 09/05/2013 07:38 AM, Peter Maydell wrote:
> 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).
> 
> Changes v1->v2:
>  * made all the CPSR_* defines unsigned for consistency
>    (suggested by rth in review)
> 
> Peter Maydell (2):
>   target-arm: Use sextract32() in branch decode
>   target-arm: Avoid "1 << 31" undefined behaviour
> 
>  target-arm/cpu.h       |   32 ++++++++++++++++----------------
>  target-arm/helper.c    |    4 ++--
>  target-arm/translate.c |    5 +++--
>  3 files changed, 21 insertions(+), 20 deletions(-)
> 

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~