Commit 7319d83a (tcg: Combine dh_is_64bit and dh_is_signed to
dh_typecode) converted the tcg type system to a 3-bit field from two
separate 1-bit fields. This subtly lost the 'signed' information from
the types as it uses the dh_alias macro to reduce the types down to
basic machine types. However, the dh_alias macro also discards sign
information, aliasing 's32' to 'i32'.

I considered two solutions; switching away from using dh_alias and
expressing typecode values for all types or staying with dh_alias and
re-inserting the sign information. The latter approach turns out a bit
cleaner as it doesn't require dealing with machine-dependent types
like 'tl'.

I re-inserted the dh_is_signed macro with its values and 'or' in that
bit to the unsigned typecode.

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

This issue was originally discovered when running snek on s390x under
qemu 6.2:

	https://github.com/keith-packard/snek/issues/58

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 include/exec/helper-head.h | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/exec/helper-head.h b/include/exec/helper-head.h
index b974eb394a..eb1066f939 100644
--- a/include/exec/helper-head.h
+++ b/include/exec/helper-head.h
@@ -85,14 +85,33 @@
 #define dh_retvar_ptr tcgv_ptr_temp(retval)
 #define dh_retvar(t) glue(dh_retvar_, dh_alias(t))
 
+#define dh_is_signed_void 0
+#define dh_is_signed_noreturn 0
+#define dh_is_signed_i32 0
+#define dh_is_signed_s32 1
+#define dh_is_signed_i64 0
+#define dh_is_signed_s64 1
+#define dh_is_signed_f16 0
+#define dh_is_signed_f32 0
+#define dh_is_signed_f64 0
+#define dh_is_signed_tl  0
+#define dh_is_signed_int 1
+/*
+ * ??? This is highly specific to the host cpu.  There are even special
+ * extension instructions that may be required, e.g. ia64's addp4.  But
+ * for now we don't support any 64-bit targets with 32-bit pointers.
+ */
+#define dh_is_signed_ptr 0
+#define dh_is_signed_cptr dh_is_signed_ptr
+#define dh_is_signed_env dh_is_signed_ptr
+#define dh_is_signed(t) dh_is_signed_##t
+
 #define dh_typecode_void 0
 #define dh_typecode_noreturn 0
 #define dh_typecode_i32 2
-#define dh_typecode_s32 3
 #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(t) (glue(dh_typecode_, dh_alias(t)) | dh_is_signed(t))
 
 #define dh_callflag_i32  0
 #define dh_callflag_s32  0
-- 
2.34.1

On 2/16/22 17:39, Keith Packard wrote:
> Commit 7319d83a (tcg: Combine dh_is_64bit and dh_is_signed to
> dh_typecode) converted the tcg type system to a 3-bit field from two
> separate 1-bit fields. This subtly lost the 'signed' information from
> the types as it uses the dh_alias macro to reduce the types down to
> basic machine types. However, the dh_alias macro also discards sign
> information, aliasing 's32' to 'i32'.

The signed information is still there, merged with the typecode:

#define dh_typecode_void 0
#define dh_typecode_noreturn 0
#define dh_typecode_i32 2
#define dh_typecode_s32 3
#define dh_typecode_i64 4
#define dh_typecode_s64 5
#define dh_typecode_ptr 6

Note that is_signed is bit 0.

But I can see that dh_alias_s32 hides it -- definitely a bug there.



r~

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

> The signed information is still there, merged with the typecode:
>
> #define dh_typecode_void 0
> #define dh_typecode_noreturn 0
> #define dh_typecode_i32 2
> #define dh_typecode_s32 3
> #define dh_typecode_i64 4
> #define dh_typecode_s64 5
> #define dh_typecode_ptr 6
>
> Note that is_signed is bit 0.
>
> But I can see that dh_alias_s32 hides it -- definitely a bug there.

Awesome. I suspected that was the underlying cause -- missing some of
what dh_alias does to the values.

As I said in the commit message, I looked at just filling out the
dh_typecode_ set to avoid using dh_alias entirely, but that actually
turned out to be a more complicated patch as you need to handle 'tl'
types, which are machine-specific; something dh_alias handles.

Either way works; I took the path which involved creating less new code
(as dh_is_signed was already written) to try and make it a bit easier to
evaluate the result.

Thanks for taking a look at the patch; I had a fine evening chasing this
down starting with a bug report written in an embedded python dialect :-)

-- 
-keith