[Qemu-devel] [PATCH] tcg: correct 32-bit tcg_gen_ld8s_i64 sign-extension

[Qemu-devel] [PATCH] tcg: correct 32-bit tcg_gen_ld8s_i64 sign-extension

[Joseph Myers]

The version of tcg_gen_ld8s_i64 for 32-bit systems does a load into
the low part of the return value - then attempts a sign extension into
the high part, but wrongly sets the high part to a sign extension of
itself rather than of the low part.  This results in TCG internal
errors from the use of the uninitialized high part (in some GCC tests
of AArch64 NEON shift intrinsics, in particular).  This patch corrects
the sign-extension logic, making it match other functions such as
tcg_gen_ld16s_i64.

Signed-off-by: Joseph Myers <joseph@codesourcery.com>

---

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index bb2bfee..43d34ea 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -790,7 +790,7 @@ void tcg_gen_ld8u_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
 void tcg_gen_ld8s_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
 {
     tcg_gen_ld8s_i32(TCGV_LOW(ret), arg2, offset);
-    tcg_gen_sari_i32(TCGV_HIGH(ret), TCGV_HIGH(ret), 31);
+    tcg_gen_sari_i32(TCGV_HIGH(ret), TCGV_LOW(ret), 31);
 }
 
 void tcg_gen_ld16u_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [Qemu-devel] [PATCH] tcg: correct 32-bit tcg_gen_ld8s_i64 sign-extension

[Peter Maydell]

On 28 October 2016 at 00:35, Joseph Myers <joseph@codesourcery.com> wrote:
> The version of tcg_gen_ld8s_i64 for 32-bit systems does a load into
> the low part of the return value - then attempts a sign extension into
> the high part, but wrongly sets the high part to a sign extension of
> itself rather than of the low part.  This results in TCG internal
> errors from the use of the uninitialized high part (in some GCC tests
> of AArch64 NEON shift intrinsics, in particular).  This patch corrects
> the sign-extension logic, making it match other functions such as
> tcg_gen_ld16s_i64.
>
> Signed-off-by: Joseph Myers <joseph@codesourcery.com>
>
> ---
>
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index bb2bfee..43d34ea 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -790,7 +790,7 @@ void tcg_gen_ld8u_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
>  void tcg_gen_ld8s_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
>  {
>      tcg_gen_ld8s_i32(TCGV_LOW(ret), arg2, offset);
> -    tcg_gen_sari_i32(TCGV_HIGH(ret), TCGV_HIGH(ret), 31);
> +    tcg_gen_sari_i32(TCGV_HIGH(ret), TCGV_LOW(ret), 31);
>  }
>
>  void tcg_gen_ld16u_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)

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

As far as I can tell this bug was introduced in commit a7812ae4123
in 2008, which is a remarkably long time for a basic bug like this
to persist...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tcg: correct 32-bit tcg_gen_ld8s_i64 sign-extension

[Richard Henderson]

On 10/28/2016 05:42 AM, Peter Maydell wrote:
> On 28 October 2016 at 00:35, Joseph Myers <joseph@codesourcery.com> wrote:
>> The version of tcg_gen_ld8s_i64 for 32-bit systems does a load into
>> the low part of the return value - then attempts a sign extension into
>> the high part, but wrongly sets the high part to a sign extension of
>> itself rather than of the low part.  This results in TCG internal
>> errors from the use of the uninitialized high part (in some GCC tests
>> of AArch64 NEON shift intrinsics, in particular).  This patch corrects
>> the sign-extension logic, making it match other functions such as
>> tcg_gen_ld16s_i64.
>>
>> Signed-off-by: Joseph Myers <joseph@codesourcery.com>
>>
>> ---
>>
>> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
>> index bb2bfee..43d34ea 100644
>> --- a/tcg/tcg-op.c
>> +++ b/tcg/tcg-op.c
>> @@ -790,7 +790,7 @@ void tcg_gen_ld8u_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
>>  void tcg_gen_ld8s_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
>>  {
>>      tcg_gen_ld8s_i32(TCGV_LOW(ret), arg2, offset);
>> -    tcg_gen_sari_i32(TCGV_HIGH(ret), TCGV_HIGH(ret), 31);
>> +    tcg_gen_sari_i32(TCGV_HIGH(ret), TCGV_LOW(ret), 31);
>>  }
>>
>>  void tcg_gen_ld16u_i64(TCGv_i64 ret, TCGv_ptr arg2, tcg_target_long offset)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> As far as I can tell this bug was introduced in commit a7812ae4123
> in 2008, which is a remarkably long time for a basic bug like this
> to persist...

Indeed, but the function is used exactly once in the entire source tree, as a 
part of target-arm's read_vec_element for aarch64.  And I suspect that 
virtually all of the aarch64 testing we've ever done has been with a 64-bit host.


r~