[Qemu-devel] [PATCH v2 0/4] tcg: out of range shift behavior

[Qemu-devel] [PATCH v2 0/4] tcg: out of range shift behavior

[Richard Henderson]

Changes v1-v2:
  * Consistently use American spelling of behaviour
  * Fix shifts use in the masking section of the optimizer as well
  * New patch that fixes optimization of deposit.

Richard Henderson (4):
  tcg: Use "unspecified behavior" for shifts
  tcg: Mask shift quantities while folding
  tcg: Mask shift counts to avoid undefined behavior
  tcg: Fix out of range shift in deposit optimizations

 tcg/README     | 18 +++++++++++++-----
 tcg/optimize.c | 45 ++++++++++++++++++++++++---------------------
 tci.c          | 20 ++++++++++----------
 3 files changed, 47 insertions(+), 36 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v2 1/4] tcg: Use "unspecified behavior" for shifts

[Richard Henderson]

Change the definition such that shifts are not allowed to crash
for any input.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/README | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tcg/README b/tcg/README
index f178212..776e925 100644
--- a/tcg/README
+++ b/tcg/README
@@ -36,6 +36,12 @@ or a memory location which is stored in a register outside QEMU TBs
 A TCG "basic block" corresponds to a list of instructions terminated
 by a branch instruction. 
 
+An operation with "undefined behavior" may result in a crash.
+
+An operation with "unspecified behavior" shall not crash.  However,
+the result may be one of several possibilities so may be considered
+an "undefined result".
+
 3) Intermediate representation
 
 3.1) Introduction
@@ -239,23 +245,25 @@ t0=t1|~t2
 
 * shl_i32/i64 t0, t1, t2
 
-t0=t1 << t2. Undefined behavior if t2 < 0 or t2 >= 32 (resp 64)
+t0=t1 << t2. Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)
 
 * shr_i32/i64 t0, t1, t2
 
-t0=t1 >> t2 (unsigned). Undefined behavior if t2 < 0 or t2 >= 32 (resp 64)
+t0=t1 >> t2 (unsigned). Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)
 
 * sar_i32/i64 t0, t1, t2
 
-t0=t1 >> t2 (signed). Undefined behavior if t2 < 0 or t2 >= 32 (resp 64)
+t0=t1 >> t2 (signed). Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)
 
 * rotl_i32/i64 t0, t1, t2
 
-Rotation of t2 bits to the left. Undefined behavior if t2 < 0 or t2 >= 32 (resp 64)
+Rotation of t2 bits to the left.
+Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)
 
 * rotr_i32/i64 t0, t1, t2
 
-Rotation of t2 bits to the right. Undefined behavior if t2 < 0 or t2 >= 32 (resp 64)
+Rotation of t2 bits to the right.
+Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)
 
 ********* Misc
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 2/4] tcg: Mask shift quantities while folding

[Richard Henderson]

The TCG result would be undefined, but we can at least produce one
plausible result and avoid triggering the wrath of analysis tools.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/optimize.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 7777743..2fb708e 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -220,34 +220,34 @@ static TCGArg do_constant_folding_2(TCGOpcode op, TCGArg x, TCGArg y)
         return x ^ y;
 
     case INDEX_op_shl_i32:
-        return (uint32_t)x << (uint32_t)y;
+        return (uint32_t)x << (y & 31);
 
     case INDEX_op_shl_i64:
-        return (uint64_t)x << (uint64_t)y;
+        return (uint64_t)x << (y & 63);
 
     case INDEX_op_shr_i32:
-        return (uint32_t)x >> (uint32_t)y;
+        return (uint32_t)x >> (y & 31);
 
     case INDEX_op_shr_i64:
-        return (uint64_t)x >> (uint64_t)y;
+        return (uint64_t)x >> (y & 63);
 
     case INDEX_op_sar_i32:
-        return (int32_t)x >> (int32_t)y;
+        return (int32_t)x >> (y & 31);
 
     case INDEX_op_sar_i64:
-        return (int64_t)x >> (int64_t)y;
+        return (int64_t)x >> (y & 63);
 
     case INDEX_op_rotr_i32:
-        return ror32(x, y);
+        return ror32(x, y & 31);
 
     case INDEX_op_rotr_i64:
-        return ror64(x, y);
+        return ror64(x, y & 63);
 
     case INDEX_op_rotl_i32:
-        return rol32(x, y);
+        return rol32(x, y & 31);
 
     case INDEX_op_rotl_i64:
-        return rol64(x, y);
+        return rol64(x, y & 63);
 
     CASE_OP_32_64(not):
         return ~x;
@@ -806,29 +806,34 @@ static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr,
 
         case INDEX_op_sar_i32:
             if (temps[args[2]].state == TCG_TEMP_CONST) {
-                mask = (int32_t)temps[args[1]].mask >> temps[args[2]].val;
+                tmp = temps[args[2]].val & 31;
+                mask = (int32_t)temps[args[1]].mask >> tmp;
             }
             break;
         case INDEX_op_sar_i64:
             if (temps[args[2]].state == TCG_TEMP_CONST) {
-                mask = (int64_t)temps[args[1]].mask >> temps[args[2]].val;
+                tmp = temps[args[2]].val & 63;
+                mask = (int64_t)temps[args[1]].mask >> tmp;
             }
             break;
 
         case INDEX_op_shr_i32:
             if (temps[args[2]].state == TCG_TEMP_CONST) {
-                mask = (uint32_t)temps[args[1]].mask >> temps[args[2]].val;
+                tmp = temps[args[2]].val & 31;
+                mask = (uint32_t)temps[args[1]].mask >> tmp;
             }
             break;
         case INDEX_op_shr_i64:
             if (temps[args[2]].state == TCG_TEMP_CONST) {
-                mask = (uint64_t)temps[args[1]].mask >> temps[args[2]].val;
+                tmp = temps[args[2]].val & 63;
+                mask = (uint64_t)temps[args[1]].mask >> tmp;
             }
             break;
 
         CASE_OP_32_64(shl):
             if (temps[args[2]].state == TCG_TEMP_CONST) {
-                mask = temps[args[1]].mask << temps[args[2]].val;
+                tmp = temps[args[2]].val & (TCG_TARGET_REG_BITS - 1);
+                mask = temps[args[1]].mask << tmp;
             }
             break;
 
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2 2/4] tcg: Mask shift quantities while folding

[Peter Maydell]

On 18 March 2014 21:30, Richard Henderson <rth@twiddle.net> wrote:
> The TCG result would be undefined, but we can at least produce one
> plausible result and avoid triggering the wrath of analysis tools.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <rth@twiddle.net>

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

-- PMM

[Qemu-devel] [PATCH v2 3/4] tcg: Mask shift counts to avoid undefined behavior

[Richard Henderson]

TCG now requires unspecified behavior rather than a potential crash,
bring the C shift within the letter of the law.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tci.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tci.c b/tci.c
index 0202ed9..6523ab8 100644
--- a/tci.c
+++ b/tci.c
@@ -669,32 +669,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             t0 = *tb_ptr++;
             t1 = tci_read_ri32(&tb_ptr);
             t2 = tci_read_ri32(&tb_ptr);
-            tci_write_reg32(t0, t1 << t2);
+            tci_write_reg32(t0, t1 << (t2 & 31));
             break;
         case INDEX_op_shr_i32:
             t0 = *tb_ptr++;
             t1 = tci_read_ri32(&tb_ptr);
             t2 = tci_read_ri32(&tb_ptr);
-            tci_write_reg32(t0, t1 >> t2);
+            tci_write_reg32(t0, t1 >> (t2 & 31));
             break;
         case INDEX_op_sar_i32:
             t0 = *tb_ptr++;
             t1 = tci_read_ri32(&tb_ptr);
             t2 = tci_read_ri32(&tb_ptr);
-            tci_write_reg32(t0, ((int32_t)t1 >> t2));
+            tci_write_reg32(t0, ((int32_t)t1 >> (t2 & 31)));
             break;
 #if TCG_TARGET_HAS_rot_i32
         case INDEX_op_rotl_i32:
             t0 = *tb_ptr++;
             t1 = tci_read_ri32(&tb_ptr);
             t2 = tci_read_ri32(&tb_ptr);
-            tci_write_reg32(t0, rol32(t1, t2));
+            tci_write_reg32(t0, rol32(t1, t2 & 31));
             break;
         case INDEX_op_rotr_i32:
             t0 = *tb_ptr++;
             t1 = tci_read_ri32(&tb_ptr);
             t2 = tci_read_ri32(&tb_ptr);
-            tci_write_reg32(t0, ror32(t1, t2));
+            tci_write_reg32(t0, ror32(t1, t2 & 31));
             break;
 #endif
 #if TCG_TARGET_HAS_deposit_i32
@@ -936,32 +936,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             t0 = *tb_ptr++;
             t1 = tci_read_ri64(&tb_ptr);
             t2 = tci_read_ri64(&tb_ptr);
-            tci_write_reg64(t0, t1 << t2);
+            tci_write_reg64(t0, t1 << (t2 & 63));
             break;
         case INDEX_op_shr_i64:
             t0 = *tb_ptr++;
             t1 = tci_read_ri64(&tb_ptr);
             t2 = tci_read_ri64(&tb_ptr);
-            tci_write_reg64(t0, t1 >> t2);
+            tci_write_reg64(t0, t1 >> (t2 & 63));
             break;
         case INDEX_op_sar_i64:
             t0 = *tb_ptr++;
             t1 = tci_read_ri64(&tb_ptr);
             t2 = tci_read_ri64(&tb_ptr);
-            tci_write_reg64(t0, ((int64_t)t1 >> t2));
+            tci_write_reg64(t0, ((int64_t)t1 >> (t2 & 63)));
             break;
 #if TCG_TARGET_HAS_rot_i64
         case INDEX_op_rotl_i64:
             t0 = *tb_ptr++;
             t1 = tci_read_ri64(&tb_ptr);
             t2 = tci_read_ri64(&tb_ptr);
-            tci_write_reg64(t0, rol64(t1, t2));
+            tci_write_reg64(t0, rol64(t1, t2 & 63));
             break;
         case INDEX_op_rotr_i64:
             t0 = *tb_ptr++;
             t1 = tci_read_ri64(&tb_ptr);
             t2 = tci_read_ri64(&tb_ptr);
-            tci_write_reg64(t0, ror64(t1, t2));
+            tci_write_reg64(t0, ror64(t1, t2 & 63));
             break;
 #endif
 #if TCG_TARGET_HAS_deposit_i64
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2 3/4] tcg: Mask shift counts to avoid undefined behavior

[Richard Henderson]

Gah.  Description should have been "tci" and cc'd the maintainer.


r~

On 03/18/2014 02:30 PM, Richard Henderson wrote:
> TCG now requires unspecified behavior rather than a potential crash,
> bring the C shift within the letter of the law.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tci.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tci.c b/tci.c
> index 0202ed9..6523ab8 100644
> --- a/tci.c
> +++ b/tci.c
> @@ -669,32 +669,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, t1 << t2);
> +            tci_write_reg32(t0, t1 << (t2 & 31));
>              break;
>          case INDEX_op_shr_i32:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, t1 >> t2);
> +            tci_write_reg32(t0, t1 >> (t2 & 31));
>              break;
>          case INDEX_op_sar_i32:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, ((int32_t)t1 >> t2));
> +            tci_write_reg32(t0, ((int32_t)t1 >> (t2 & 31)));
>              break;
>  #if TCG_TARGET_HAS_rot_i32
>          case INDEX_op_rotl_i32:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, rol32(t1, t2));
> +            tci_write_reg32(t0, rol32(t1, t2 & 31));
>              break;
>          case INDEX_op_rotr_i32:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, ror32(t1, t2));
> +            tci_write_reg32(t0, ror32(t1, t2 & 31));
>              break;
>  #endif
>  #if TCG_TARGET_HAS_deposit_i32
> @@ -936,32 +936,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, t1 << t2);
> +            tci_write_reg64(t0, t1 << (t2 & 63));
>              break;
>          case INDEX_op_shr_i64:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, t1 >> t2);
> +            tci_write_reg64(t0, t1 >> (t2 & 63));
>              break;
>          case INDEX_op_sar_i64:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, ((int64_t)t1 >> t2));
> +            tci_write_reg64(t0, ((int64_t)t1 >> (t2 & 63)));
>              break;
>  #if TCG_TARGET_HAS_rot_i64
>          case INDEX_op_rotl_i64:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, rol64(t1, t2));
> +            tci_write_reg64(t0, rol64(t1, t2 & 63));
>              break;
>          case INDEX_op_rotr_i64:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, ror64(t1, t2));
> +            tci_write_reg64(t0, ror64(t1, t2 & 63));
>              break;
>  #endif
>  #if TCG_TARGET_HAS_deposit_i64
> 

Re: [Qemu-devel] [PATCH v2 3/4] tcg: Mask shift counts to avoid undefined behavior

[Stefan Weil]

Am 18.03.2014 22:30, schrieb Richard Henderson:
> TCG now requires unspecified behavior rather than a potential crash,
> bring the C shift within the letter of the law.

I know that C does not define the result of some shift / rotate
operations, but I don't understand the sentence above. Why does TCG or
TCI require unspecified behaviour now? Where was or is a potential crash?

The modifications below won't harm, but make the TCG interpreter slower.
Are they (all) necessary? Are there test cases which fail with the old code?

> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tci.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/tci.c b/tci.c
> index 0202ed9..6523ab8 100644
> --- a/tci.c
> +++ b/tci.c
> @@ -669,32 +669,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, t1 << t2);
> +            tci_write_reg32(t0, t1 << (t2 & 31));
>              break;
>          case INDEX_op_shr_i32:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, t1 >> t2);
> +            tci_write_reg32(t0, t1 >> (t2 & 31));

Right shifts of unsigned values with unsigned shift count are always
defined, aren't they? So masking for those cases should not be needed.

>              break;
>          case INDEX_op_sar_i32:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, ((int32_t)t1 >> t2));
> +            tci_write_reg32(t0, ((int32_t)t1 >> (t2 & 31)));
>              break;
>  #if TCG_TARGET_HAS_rot_i32
>          case INDEX_op_rotl_i32:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, rol32(t1, t2));
> +            tci_write_reg32(t0, rol32(t1, t2 & 31));

What about other users of rol32?

>              break;
>          case INDEX_op_rotr_i32:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri32(&tb_ptr);
>              t2 = tci_read_ri32(&tb_ptr);
> -            tci_write_reg32(t0, ror32(t1, t2));
> +            tci_write_reg32(t0, ror32(t1, t2 & 31));
>              break;
>  #endif
>  #if TCG_TARGET_HAS_deposit_i32
> @@ -936,32 +936,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, t1 << t2);
> +            tci_write_reg64(t0, t1 << (t2 & 63));
>              break;
>          case INDEX_op_shr_i64:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, t1 >> t2);
> +            tci_write_reg64(t0, t1 >> (t2 & 63));
>              break;
>          case INDEX_op_sar_i64:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, ((int64_t)t1 >> t2));
> +            tci_write_reg64(t0, ((int64_t)t1 >> (t2 & 63)));
>              break;
>  #if TCG_TARGET_HAS_rot_i64
>          case INDEX_op_rotl_i64:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, rol64(t1, t2));
> +            tci_write_reg64(t0, rol64(t1, t2 & 63));
>              break;
>          case INDEX_op_rotr_i64:
>              t0 = *tb_ptr++;
>              t1 = tci_read_ri64(&tb_ptr);
>              t2 = tci_read_ri64(&tb_ptr);
> -            tci_write_reg64(t0, ror64(t1, t2));
> +            tci_write_reg64(t0, ror64(t1, t2 & 63));
>              break;
>  #endif
>  #if TCG_TARGET_HAS_deposit_i64
> 

Regards
Stefan

Re: [Qemu-devel] [PATCH v2 3/4] tcg: Mask shift counts to avoid undefined behavior

[Peter Maydell]

On 19 March 2014 06:21, Stefan Weil <sw@weilnetz.de> wrote:
> Am 18.03.2014 22:30, schrieb Richard Henderson:
>> TCG now requires unspecified behavior rather than a potential crash,
>> bring the C shift within the letter of the law.
>
> I know that C does not define the result of some shift / rotate
> operations, but I don't understand the sentence above. Why does TCG or
> TCI require unspecified behaviour now? Where was or is a potential crash?

The specific example we hit is that for variable shifts by a value
which happens to be zero, the x86 frontend will emit a shift
by -1. (It doesn't subsequently use that result because there's
a later movcond in the TCG op sequence.)

So we had a choice of:
 (a) make the x86 frontend slower and more awkward (and perhaps
      also other frontends which did the same thing, if any)
 (b) tighten the definition of the TCG shift ops from "undefined
    behaviour, may crash" to "unspecified behaviour, must produce
    a result but it could be anything"

and since the latter isn't a problem for the backends where
performance is important (ie the native ones), because
the hardware always acts like 'unknown result' rather than
'might blow up' (b) is a clearly better choice.

> The modifications below won't harm, but make the TCG interpreter slower.
> Are they (all) necessary? Are there test cases which fail with the old code?

If you want to try to find them you could run with clang
-fsanitize=undefined. (Try 'make check', the code we run
during the ACPI test does this.)

>>
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> ---
>>  tci.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/tci.c b/tci.c
>> index 0202ed9..6523ab8 100644
>> --- a/tci.c
>> +++ b/tci.c
>> @@ -669,32 +669,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>>              t0 = *tb_ptr++;
>>              t1 = tci_read_ri32(&tb_ptr);
>>              t2 = tci_read_ri32(&tb_ptr);
>> -            tci_write_reg32(t0, t1 << t2);
>> +            tci_write_reg32(t0, t1 << (t2 & 31));
>>              break;
>>          case INDEX_op_shr_i32:
>>              t0 = *tb_ptr++;
>>              t1 = tci_read_ri32(&tb_ptr);
>>              t2 = tci_read_ri32(&tb_ptr);
>> -            tci_write_reg32(t0, t1 >> t2);
>> +            tci_write_reg32(t0, t1 >> (t2 & 31));
>
> Right shifts of unsigned values with unsigned shift count are always
> defined, aren't they?

No. The C standard says "If the value of the right operand is
negative or is greater than or equal to the width of the promoted
left operand, the behavior is undefined." for all shifts.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/4] tcg: Mask shift counts to avoid undefined behavior

[Richard Henderson]

On 03/18/2014 11:21 PM, Stefan Weil wrote:
> Are there test cases which fail with the old code?

Only when running under analysis tools looking for technical violations of the
standard.  Under normal circumstances it would never crash since, as with the
native backends, the underlying cpu is going to treat this some rational way.

But I sincerely doubt that we could measure any performance loss in TCI.  The
single extra arithmetic operation should be swallowed by the loop back to
interpret the next opcode.


r~

Re: [Qemu-devel] [PATCH v2 3/4] tcg: Mask shift counts to avoid undefined behavior

[Peter Maydell]

On 19 March 2014 15:25, Richard Henderson <rth@twiddle.net> wrote:
> On 03/18/2014 11:21 PM, Stefan Weil wrote:
>> Are there test cases which fail with the old code?
>
> Only when running under analysis tools looking for technical violations of the
> standard.  Under normal circumstances it would never crash since, as with the
> native backends, the underlying cpu is going to treat this some rational way.

I think the major reason for fixing this kind of thing is that
modern C compilers are becoming increasingly adversarial,
and if we don't try to clean up our use of undefined behaviours
one day gcc is going to decide that it can optimize our code
into something that breaks...

thanks
-- PMM

[Qemu-devel] [PATCH v2 4/4] tcg: Fix out of range shift in deposit optimizations

[Richard Henderson]

By inspection, for a deposit(x, y, 0, 64), we'd have a shift of (1<<64)
and everything else falls apart.  But we can reuse the existing deposit
logic to get this right.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/optimize.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 2fb708e..c447062 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -843,9 +843,8 @@ static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr,
             break;
 
         CASE_OP_32_64(deposit):
-            tmp = ((1ull << args[4]) - 1);
-            mask = ((temps[args[1]].mask & ~(tmp << args[3]))
-                    | ((temps[args[2]].mask & tmp) << args[3]));
+            mask = deposit64(temps[args[1]].mask, args[3], args[4],
+                             temps[args[2]].mask);
             break;
 
         CASE_OP_32_64(or):
@@ -1060,9 +1059,8 @@ static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr,
             if (temps[args[1]].state == TCG_TEMP_CONST
                 && temps[args[2]].state == TCG_TEMP_CONST) {
                 s->gen_opc_buf[op_index] = op_to_movi(op);
-                tmp = ((1ull << args[4]) - 1);
-                tmp = (temps[args[1]].val & ~(tmp << args[3]))
-                      | ((temps[args[2]].val & tmp) << args[3]);
+                tmp = deposit64(temps[args[1]].val, args[3], args[4],
+                                temps[args[2]].val);
                 tcg_opt_gen_movi(gen_args, args[0], tmp);
                 gen_args += 2;
                 args += 5;
-- 
1.8.5.3