[PATCH] tcg: Remove dh_alias indirection for dh_typecode

[PATCH] tcg: Remove dh_alias indirection for dh_typecode

[Richard Henderson]

The dh_alias redirect is intended to handle TCG types as distinguished
from C types.  TCG does not distinguish signed int from unsigned int,
because they are the same size.  However, we need to retain this
distinction for dh_typecode, lest we fail to extend abi types properly
for the host call parameters.

This bug was detected when running the 'arm' emulator on an s390
system. The s390 uses TCG_TARGET_EXTEND_ARGS which triggers code
in tcg_gen_callN to extend 32 bit values to 64 bits; the incorrect
sign data in the typemask for each argument caused the values to be
extended as unsigned values.

This simple program exhibits the problem:

	static volatile int num = -9;
	static volatile int den = -5;

	int
	main(void)
	{
		int quo = num / den;
		printf("num %d den %d quo %d\n", num, den, quo);
		exit(0);
	}

When run on the broken qemu, this results in:

	num -9 den -5 quo 0

The correct result is:

	num -9 den -5 quo 1

Reported-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/helper-head.h   | 19 ++++++++++---------
 target/hppa/helper.h         |  2 ++
 target/i386/ops_sse_header.h |  3 +++
 target/m68k/helper.h         |  1 +
 target/ppc/helper.h          |  3 +++
 5 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/exec/helper-head.h b/include/exec/helper-head.h
index b974eb394a..734af067fe 100644
--- a/include/exec/helper-head.h
+++ b/include/exec/helper-head.h
@@ -53,13 +53,16 @@
 # ifdef TARGET_LONG_BITS
 #  if TARGET_LONG_BITS == 32
 #   define dh_alias_tl i32
+#   define dh_typecode_tl dh_typecode_i32
 #  else
 #   define dh_alias_tl i64
+#   define dh_typecode_tl dh_typecode_i64
 #  endif
 # endif
-# define dh_alias_env ptr
 # define dh_ctype_tl target_ulong
+# define dh_alias_env ptr
 # define dh_ctype_env CPUArchState *
+# define dh_typecode_env dh_typecode_ptr
 #endif
 
 /* We can't use glue() here because it falls foul of C preprocessor
@@ -92,18 +95,16 @@
 #define dh_typecode_i64 4
 #define dh_typecode_s64 5
 #define dh_typecode_ptr 6
-#define dh_typecode(t) glue(dh_typecode_, dh_alias(t))
+#define dh_typecode_int dh_typecode_s32
+#define dh_typecode_f16 dh_typecode_i32
+#define dh_typecode_f32 dh_typecode_i32
+#define dh_typecode_f64 dh_typecode_i64
+#define dh_typecode_cptr dh_typecode_ptr
+#define dh_typecode(t) dh_typecode_##t
 
 #define dh_callflag_i32  0
-#define dh_callflag_s32  0
-#define dh_callflag_int  0
 #define dh_callflag_i64  0
-#define dh_callflag_s64  0
-#define dh_callflag_f16  0
-#define dh_callflag_f32  0
-#define dh_callflag_f64  0
 #define dh_callflag_ptr  0
-#define dh_callflag_cptr dh_callflag_ptr
 #define dh_callflag_void 0
 #define dh_callflag_noreturn TCG_CALL_NO_RETURN
 #define dh_callflag(t) glue(dh_callflag_, dh_alias(t))
diff --git a/target/hppa/helper.h b/target/hppa/helper.h
index fe8a9ce493..c7e35ce8c7 100644
--- a/target/hppa/helper.h
+++ b/target/hppa/helper.h
@@ -1,7 +1,9 @@
 #if TARGET_REGISTER_BITS == 64
 # define dh_alias_tr     i64
+# define dh_typecode_tr  dh_typecode_i64
 #else
 # define dh_alias_tr     i32
+# define dh_typecode_tr  dh_typecode_i32
 #endif
 #define dh_ctype_tr      target_ureg
 
diff --git a/target/i386/ops_sse_header.h b/target/i386/ops_sse_header.h
index e68af5c403..cef28f2aae 100644
--- a/target/i386/ops_sse_header.h
+++ b/target/i386/ops_sse_header.h
@@ -30,6 +30,9 @@
 #define dh_ctype_Reg Reg *
 #define dh_ctype_ZMMReg ZMMReg *
 #define dh_ctype_MMXReg MMXReg *
+#define dh_typecode_Reg dh_typecode_ptr
+#define dh_typecode_ZMMReg dh_typecode_ptr
+#define dh_typecode_MMXReg dh_typecode_ptr
 
 DEF_HELPER_3(glue(psrlw, SUFFIX), void, env, Reg, Reg)
 DEF_HELPER_3(glue(psraw, SUFFIX), void, env, Reg, Reg)
diff --git a/target/m68k/helper.h b/target/m68k/helper.h
index 9842eeaa95..0a6b4146f6 100644
--- a/target/m68k/helper.h
+++ b/target/m68k/helper.h
@@ -17,6 +17,7 @@ DEF_HELPER_4(cas2l_parallel, void, env, i32, i32, i32)
 
 #define dh_alias_fp ptr
 #define dh_ctype_fp FPReg *
+#define dh_typecode_fp dh_typecode_ptr
 
 DEF_HELPER_3(exts32, void, env, fp, s32)
 DEF_HELPER_3(extf32, void, env, fp, f32)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index ab008c9d4e..ae7d503fcf 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -127,9 +127,11 @@ DEF_HELPER_FLAGS_1(ftsqrt, TCG_CALL_NO_RWG_SE, i32, i64)
 
 #define dh_alias_avr ptr
 #define dh_ctype_avr ppc_avr_t *
+#define dh_typecode_avr dh_typecode_ptr
 
 #define dh_alias_vsr ptr
 #define dh_ctype_vsr ppc_vsr_t *
+#define dh_typecode_vsr dh_typecode_ptr
 
 DEF_HELPER_3(vavgub, void, avr, avr, avr)
 DEF_HELPER_3(vavguh, void, avr, avr, avr)
@@ -708,6 +710,7 @@ DEF_HELPER_3(store_dbatu, void, env, i32, tl)
 
 #define dh_alias_fprp ptr
 #define dh_ctype_fprp ppc_fprp_t *
+#define dh_typecode_fprp dh_typecode_ptr
 
 DEF_HELPER_4(DADD, void, env, fprp, fprp, fprp)
 DEF_HELPER_4(DADDQ, void, env, fprp, fprp, fprp)
-- 
2.25.1

Re: [PATCH] tcg: Remove dh_alias indirection for dh_typecode

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> The dh_alias redirect is intended to handle TCG types as distinguished
> from C types.  TCG does not distinguish signed int from unsigned int,
> because they are the same size.  However, we need to retain this
> distinction for dh_typecode, lest we fail to extend abi types properly
> for the host call parameters.
>
> This bug was detected when running the 'arm' emulator on an s390
> system. The s390 uses TCG_TARGET_EXTEND_ARGS which triggers code
> in tcg_gen_callN to extend 32 bit values to 64 bits; the incorrect
> sign data in the typemask for each argument caused the values to be
> extended as unsigned values.
>
> This simple program exhibits the problem:
>
> 	static volatile int num = -9;
> 	static volatile int den = -5;
>
> 	int
> 	main(void)
> 	{
> 		int quo = num / den;
> 		printf("num %d den %d quo %d\n", num, den, quo);
> 		exit(0);
> 	}
>
> When run on the broken qemu, this results in:
>
> 	num -9 den -5 quo 0
>
> The correct result is:
>
> 	num -9 den -5 quo 1
>

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/876

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH] tcg: Remove dh_alias indirection for dh_typecode

[Christian Ehrhardt]

On Thu, Feb 17, 2022 at 4:48 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The dh_alias redirect is intended to handle TCG types as distinguished
> from C types.  TCG does not distinguish signed int from unsigned int,
> because they are the same size.  However, we need to retain this
> distinction for dh_typecode, lest we fail to extend abi types properly
> for the host call parameters.

Thank you Richard and Keith for the fix for
- https://github.com/keith-packard/snek/issues/58
- https://gitlab.com/qemu-project/qemu/-/issues/876

I did apply that and tested it on s390x with the load that originally found it.
With qemu 6.2 + that patch I ran the full testsuite of snek-arm on
s390x and it works great now.
If you like feel free to add any combination of the following when committing:

Fixes: #876
Fixes: 7319d83a735 ("tcg: Combine dh_is_64bit and dh_is_signed to dh_typecode")
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

> This bug was detected when running the 'arm' emulator on an s390
> system. The s390 uses TCG_TARGET_EXTEND_ARGS which triggers code
> in tcg_gen_callN to extend 32 bit values to 64 bits; the incorrect
> sign data in the typemask for each argument caused the values to be
> extended as unsigned values.
>
> This simple program exhibits the problem:
>
>         static volatile int num = -9;
>         static volatile int den = -5;
>
>         int
>         main(void)
>         {
>                 int quo = num / den;
>                 printf("num %d den %d quo %d\n", num, den, quo);
>                 exit(0);
>         }
>
> When run on the broken qemu, this results in:
>
>         num -9 den -5 quo 0
>
> The correct result is:
>
>         num -9 den -5 quo 1
>
> Reported-by: Keith Packard <keithp@keithp.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/helper-head.h   | 19 ++++++++++---------
>  target/hppa/helper.h         |  2 ++
>  target/i386/ops_sse_header.h |  3 +++
>  target/m68k/helper.h         |  1 +
>  target/ppc/helper.h          |  3 +++
>  5 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/include/exec/helper-head.h b/include/exec/helper-head.h
> index b974eb394a..734af067fe 100644
> --- a/include/exec/helper-head.h
> +++ b/include/exec/helper-head.h
> @@ -53,13 +53,16 @@
>  # ifdef TARGET_LONG_BITS
>  #  if TARGET_LONG_BITS == 32
>  #   define dh_alias_tl i32
> +#   define dh_typecode_tl dh_typecode_i32
>  #  else
>  #   define dh_alias_tl i64
> +#   define dh_typecode_tl dh_typecode_i64
>  #  endif
>  # endif
> -# define dh_alias_env ptr
>  # define dh_ctype_tl target_ulong
> +# define dh_alias_env ptr
>  # define dh_ctype_env CPUArchState *
> +# define dh_typecode_env dh_typecode_ptr
>  #endif
>
>  /* We can't use glue() here because it falls foul of C preprocessor
> @@ -92,18 +95,16 @@
>  #define dh_typecode_i64 4
>  #define dh_typecode_s64 5
>  #define dh_typecode_ptr 6
> -#define dh_typecode(t) glue(dh_typecode_, dh_alias(t))
> +#define dh_typecode_int dh_typecode_s32
> +#define dh_typecode_f16 dh_typecode_i32
> +#define dh_typecode_f32 dh_typecode_i32
> +#define dh_typecode_f64 dh_typecode_i64
> +#define dh_typecode_cptr dh_typecode_ptr
> +#define dh_typecode(t) dh_typecode_##t
>
>  #define dh_callflag_i32  0
> -#define dh_callflag_s32  0
> -#define dh_callflag_int  0
>  #define dh_callflag_i64  0
> -#define dh_callflag_s64  0
> -#define dh_callflag_f16  0
> -#define dh_callflag_f32  0
> -#define dh_callflag_f64  0
>  #define dh_callflag_ptr  0
> -#define dh_callflag_cptr dh_callflag_ptr
>  #define dh_callflag_void 0
>  #define dh_callflag_noreturn TCG_CALL_NO_RETURN
>  #define dh_callflag(t) glue(dh_callflag_, dh_alias(t))
> diff --git a/target/hppa/helper.h b/target/hppa/helper.h
> index fe8a9ce493..c7e35ce8c7 100644
> --- a/target/hppa/helper.h
> +++ b/target/hppa/helper.h
> @@ -1,7 +1,9 @@
>  #if TARGET_REGISTER_BITS == 64
>  # define dh_alias_tr     i64
> +# define dh_typecode_tr  dh_typecode_i64
>  #else
>  # define dh_alias_tr     i32
> +# define dh_typecode_tr  dh_typecode_i32
>  #endif
>  #define dh_ctype_tr      target_ureg
>
> diff --git a/target/i386/ops_sse_header.h b/target/i386/ops_sse_header.h
> index e68af5c403..cef28f2aae 100644
> --- a/target/i386/ops_sse_header.h
> +++ b/target/i386/ops_sse_header.h
> @@ -30,6 +30,9 @@
>  #define dh_ctype_Reg Reg *
>  #define dh_ctype_ZMMReg ZMMReg *
>  #define dh_ctype_MMXReg MMXReg *
> +#define dh_typecode_Reg dh_typecode_ptr
> +#define dh_typecode_ZMMReg dh_typecode_ptr
> +#define dh_typecode_MMXReg dh_typecode_ptr
>
>  DEF_HELPER_3(glue(psrlw, SUFFIX), void, env, Reg, Reg)
>  DEF_HELPER_3(glue(psraw, SUFFIX), void, env, Reg, Reg)
> diff --git a/target/m68k/helper.h b/target/m68k/helper.h
> index 9842eeaa95..0a6b4146f6 100644
> --- a/target/m68k/helper.h
> +++ b/target/m68k/helper.h
> @@ -17,6 +17,7 @@ DEF_HELPER_4(cas2l_parallel, void, env, i32, i32, i32)
>
>  #define dh_alias_fp ptr
>  #define dh_ctype_fp FPReg *
> +#define dh_typecode_fp dh_typecode_ptr
>
>  DEF_HELPER_3(exts32, void, env, fp, s32)
>  DEF_HELPER_3(extf32, void, env, fp, f32)
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index ab008c9d4e..ae7d503fcf 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -127,9 +127,11 @@ DEF_HELPER_FLAGS_1(ftsqrt, TCG_CALL_NO_RWG_SE, i32, i64)
>
>  #define dh_alias_avr ptr
>  #define dh_ctype_avr ppc_avr_t *
> +#define dh_typecode_avr dh_typecode_ptr
>
>  #define dh_alias_vsr ptr
>  #define dh_ctype_vsr ppc_vsr_t *
> +#define dh_typecode_vsr dh_typecode_ptr
>
>  DEF_HELPER_3(vavgub, void, avr, avr, avr)
>  DEF_HELPER_3(vavguh, void, avr, avr, avr)
> @@ -708,6 +710,7 @@ DEF_HELPER_3(store_dbatu, void, env, i32, tl)
>
>  #define dh_alias_fprp ptr
>  #define dh_ctype_fprp ppc_fprp_t *
> +#define dh_typecode_fprp dh_typecode_ptr
>
>  DEF_HELPER_4(DADD, void, env, fprp, fprp, fprp)
>  DEF_HELPER_4(DADDQ, void, env, fprp, fprp, fprp)
> --
> 2.25.1
>
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] tcg: Remove dh_alias indirection for dh_typecode

[Keith Packard via]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

Richard Henderson <richard.henderson@linaro.org> writes:

> Reported-by: Keith Packard <keithp@keithp.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Looks good to me, and it passes my very simple test when run on s390.

Tested-by: Keith Packard <keithp@keithp.com>

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]