qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Keith Packard via <qemu-devel@nongnu.org>
To: qemu-devel@nongnu.org
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Keith Packard <keithp@keithp.com>
Subject: [PATCH] tcg: Add 'signed' bit to typecodes
Date: Tue, 15 Feb 2022 22:39:45 -0800	[thread overview]
Message-ID: <20220216063945.1986257-1-keithp@keithp.com> (raw)

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



             reply	other threads:[~2022-02-16  9:10 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-16  6:39 Keith Packard via [this message]
2022-02-16 21:13 ` [PATCH] tcg: Add 'signed' bit to typecodes Richard Henderson
2022-02-16 21:48   ` Keith Packard via

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220216063945.1986257-1-keithp@keithp.com \
    --to=qemu-devel@nongnu.org \
    --cc=keithp@keithp.com \
    --cc=pbonzini@redhat.com \
    --cc=richard.henderson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).