[Qemu-devel] [PATCH 0/3] tcg: out of range shift behaviour

[Qemu-devel] [PATCH 0/3] tcg: out of range shift behaviour

[Richard Henderson]

Following on from this morning's discussion, and, apparently,
another one from last year.


r~


Richard Henderson (3):
  tcg: Mask shift quantities while folding
  tcg: Use "unspecified behaviour" for shifts
  tcg: Mask shift counts to avoid undefined behaviour

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

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/3] 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 7777743..2fc6344 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;
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 1/3] tcg: Mask shift quantities while folding

[Peter Maydell]

On 18 March 2014 15:48, 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>

Don't you also need to do something similar for the
"calculate known-zeroes bits" code on lines 807..833 ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] tcg: Mask shift quantities while folding

[Richard Henderson]

On 03/18/2014 08:59 AM, Peter Maydell wrote:
> Don't you also need to do something similar for the
> "calculate known-zeroes bits" code on lines 807..833 ?

Yep.


r~

[Qemu-devel] [PATCH 2/3] tcg: Use "unspecified behaviour" for shifts

[Richard Henderson]

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

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..431cee7 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 behaviour" 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

Re: [Qemu-devel] [PATCH 2/3] tcg: Use "unspecified behaviour" for shifts

[Peter Maydell]

On 18 March 2014 15:48, Richard Henderson <rth@twiddle.net> wrote:
> Change the definition such that shifts are not allowed to crash
> for any input.
>
> 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..431cee7 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 behaviour" shall not crash.  However,

no 'u' would be consistent with the US spelling elsewhere.

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


Incidentally, are deposit ops with bogus pos and len
undefined or unspecified behavior? (And is deposit of
64 bits to bit 0 allowed?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] tcg: Use "unspecified behaviour" for shifts

[Richard Henderson]

On 03/18/2014 09:02 AM, Peter Maydell wrote:
> On 18 March 2014 15:48, Richard Henderson <rth@twiddle.net> wrote:
>> Change the definition such that shifts are not allowed to crash
>> for any input.
>>
>> 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..431cee7 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 behaviour" shall not crash.  However,
> 
> no 'u' would be consistent with the US spelling elsewhere.

Oops, by fingers have no consistency.

> Incidentally, are deposit ops with bogus pos and len
> undefined or unspecified behavior? (And is deposit of
> 64 bits to bit 0 allowed?)

Yes, deposit of 64 at 0 is allowed.

Since deposit parameters are immediate constants instead
of values in TCGv temporaries, we check them at opcode
creation in tcg-op.h.

Since we abort that's undefined, I guess.


r~

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

[Richard Henderson]

TCG now requires unspecified behaviour 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