Re: [hatari-devel] undefined behaviour fixes

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

> Am 05.01.2025 um 12:39 schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:
> 
> 
>> Am 04.01.2025 um 18:07 schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:
>> 
>> 
>>> Am 31.12.2024 um 18:24 schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:
>>> 
>>> Hello Toni,
>>> 
>>> here is a little patch that should fix some undefined behaviour (getting these from runtime checking). Please review before applying the patch.
>>> 
>>> There are many more warnings from cpuemu_XX files. But I don’t know how to fix those.
>>> 
>>> Kind regards,
>>> Andreas
>>> 
>>> <cpu_warnings.txt>
>> 
>> I found some more. An updated patch is appended.
>> 
>> <cpu_warnings2.txt>
> 
> Another patch is appended. Now I also fixed the problems in the cpuemu_XX files. Please note that I only use cpuemu_31 and cpuemu32, so others are untested. It seems that all is working normally. But anyway I think some heavy review is needed, especially for the changes in gencpu.
> 
> I really think these issues need to be fixed, because with the current code undefined behaviour can occur in many critical functions.
> 
> Good news for Hatari: Although I tested only with Previous, no undefined behaviour was not detected in any file that was derived from Hatari, including all DSP code.
> 
> <cpu_warnings3.txt>

I’ve run into problems with my last patch. So here is a new one. There is quite some type chaos inside the CPU code. I get lots of undefined signed integer overflow, shifting with values greater than type size, shifting negative values, shifting with negative shifts, etc. Not sure this fixes all of them, but at least I no longer run into one of these immediately.

diff -ru a/src/cpu/cpummu030.c b/src/cpu/cpummu030.c
--- a/src/cpu/cpummu030.c	2024-10-27 15:30:07
+++ b/src/cpu/cpummu030.c	2025-01-05 16:50:11
@@ -910,7 +910,7 @@
         mmu030.translation.table[i].shift = shift;
         /* Build the mask */
         for (j = 0; j < TI_bits[i]; j++) {
-            mmu030.translation.table[i].mask |= (1<<(mmu030.translation.table[i].shift + j));
+            mmu030.translation.table[i].mask |= ((uae_u32)1<<(mmu030.translation.table[i].shift + j));
         }
         /* Update until reaching the last table */
         mmu030.translation.last_table = i;
@@ -1799,10 +1799,10 @@
 	if (!data)
 		return;
 	int reg = data & 7;
-	int adj = (data & (1 << 5)) ? -1 : 1;
+	uae_s32 adj = (data & (1 << 5)) ? -1 : 1;
 	if (dir)
 		adj = -adj;
-	adj <<= (data >> 3) & 3;
+	adj = (uae_s32)((uae_u32)adj << ((data >> 3) & 3));
 	m68k_areg(regs, reg) += adj;
 	if (idx >= 0) {
 		struct mmufixup *m = &mmufixup[idx];
@@ -3139,7 +3139,7 @@
 	uae_s32 regd = regs.regs[reg];
 	if ((dp & 0x800) == 0)
 		regd = (uae_s32)(uae_s16)regd;
-	regd <<= (dp >> 9) & 3;
+	regd = (uae_s32)((uae_u32)regd << ((dp >> 9) & 3));
 	if (dp & 0x100) {
 		uae_s32 outer = 0;
 		if (dp & 0x80)
diff -ru a/src/cpu/fpp_softfloat.c b/src/cpu/fpp_softfloat.c
--- a/src/cpu/fpp_softfloat.c	2022-12-29 16:51:59
+++ b/src/cpu/fpp_softfloat.c	2024-12-31 13:52:10
@@ -229,7 +229,7 @@
 static void from_exten(fpdata *fpd, uae_u32 *wrd1, uae_u32 *wrd2, uae_u32 *wrd3)
 {
 	floatx80 f = floatx80_to_floatx80(fpd->fpx, &fs);
-	*wrd1 = (uae_u32)(f.high << 16);
+	*wrd1 = (uae_u32)f.high << 16;
 	*wrd2 = f.low >> 32;
 	*wrd3 = (uae_u32)f.low;
 }
@@ -241,7 +241,7 @@
 }
 static void from_exten_fmovem(fpdata *fpd, uae_u32 *wrd1, uae_u32 *wrd2, uae_u32 *wrd3)
  {
-	*wrd1 = (uae_u32)(fpd->fpx.high << 16);
+	*wrd1 = (uae_u32)fpd->fpx.high << 16;
 	*wrd2 = fpd->fpx.low >> 32;
 	*wrd3 = (uae_u32)fpd->fpx.low;
  }
diff -ru a/src/cpu/gencpu.c b/src/cpu/gencpu.c
--- a/src/cpu/gencpu.c	2024-05-04 20:10:03
+++ b/src/cpu/gencpu.c	2025-01-06 10:29:11
@@ -5758,7 +5758,7 @@
 		genamodedual(curi,
 			curi->smode, "srcreg", curi->size, "src", 1, 0,
 			curi->dmode, "dstreg", sz_long, "dst", 1, GF_RMW);
-		out("uae_u32 newv = dst - src;\n");
+		out("uae_u32 newv = (uae_u32)(uae_s32)dst - (uae_u32)(uae_s32)src;\n");
 		if (curi->smode == immi) {
 			// SUBAQ.x is always 8 cycles
 			c += 4;
@@ -5788,7 +5788,7 @@
 		}
 		genamode(curi, curi->smode, "srcreg", curi->size, "src", 1, 0, GF_AA | GF_REVERSE);
 		genamode(curi, curi->dmode, "dstreg", curi->size, "dst", 1, 0, GF_AA | GF_REVERSE | GF_RMW | GF_SECONDEA | (curi->size == sz_long && !isreg(curi->smode) ? GF_IPL | GF_IPLMID : 0));
-		out("uae_u32 newv = dst - src - (GET_XFLG() ? 1 : 0);\n");
+		out("uae_u32 newv = (uae_u32)(uae_s32)dst - (uae_u32)(uae_s32)src - (GET_XFLG() ? 1 : 0);\n");
 		if (cpu_level >= 2) {
 			genflags(flag_subx, curi->size, "newv", "src", "dst");
 			genflags(flag_zn, curi->size, "newv", "", "");
@@ -5974,7 +5974,7 @@
 		genamodedual(curi,
 			curi->smode, "srcreg", curi->size, "src", 1, 0,
 			curi->dmode, "dstreg", sz_long, "dst", 1, GF_RMW);
-		out("uae_u32 newv = dst + src;\n");
+		out("uae_u32 newv = (uae_u32)(uae_s32)dst + (uae_u32)(uae_s32)src;\n");
 		if (curi->smode == immi) {
 			// ADDAQ.x is always 8 cycles
 			c += 4;
@@ -6004,7 +6004,7 @@
 		}
 		genamode(curi, curi->smode, "srcreg", curi->size, "src", 1, 0, GF_AA | GF_REVERSE);
 		genamode(curi, curi->dmode, "dstreg", curi->size, "dst", 1, 0, GF_AA | GF_REVERSE | GF_RMW | GF_SECONDEA | (curi->size == sz_long && !isreg(curi->smode) ? GF_IPL | GF_IPLMID : 0));
-		out("uae_u32 newv = dst + src + (GET_XFLG() ? 1 : 0);\n");
+		out("uae_u32 newv = (uae_u32)(uae_s32)dst + (uae_u32)(uae_s32)src + (GET_XFLG() ? 1 : 0);\n");
 		if (cpu_level >= 2) {
 			genflags(flag_addx, curi->size, "newv", "src", "dst");
 			genflags(flag_zn, curi->size, "newv", "", "");
@@ -6147,7 +6147,7 @@
 		break;
 	case i_NEGX:
 		genamode(curi, curi->smode, "srcreg", curi->size, "src", 1, 0, GF_RMW);
-		out("uae_u32 newv = 0 - src - (GET_XFLG() ? 1 : 0);\n");
+		out("uae_u32 newv = (uae_u32)0 - (uae_u32)(uae_s32)src - (GET_XFLG() ? 1 : 0);\n");
 		genflags(flag_subx, curi->size, "newv", "src", "0");
 		genflags(flag_zn, curi->size, "newv", "", "");
 		if (curi->smode == Dreg) {
@@ -6398,14 +6398,14 @@
 			next_level_000();
 		}
 		if (curi->mnemo == i_BCHG) {
-			out("dst ^= (1 << src);\n");
-			out("SET_ZFLG(((uae_u32)dst & (1 << src)) >> src);\n");
+			out("dst ^= ((uae_u32)1 << src);\n");
+			out("SET_ZFLG(((uae_u32)dst & ((uae_u32)1 << src)) >> src);\n");
 		} else if (curi->mnemo == i_BCLR) {
 			out("SET_ZFLG(1 ^ ((dst >> src) & 1));\n");
-			out("dst &= ~(1 << src);\n");
+			out("dst &= ~((uae_u32)1 << src);\n");
 		} else if (curi->mnemo == i_BSET) {
 			out("SET_ZFLG(1 ^ ((dst >> src) & 1));\n");
-			out("dst |= (1 << src);\n");
+			out("dst |= ((uae_u32)1 << src);\n");
 		}
 		if (curi->size != sz_long) {
 			if (curi->smode < imm && curi->smode != Dreg) {
@@ -6923,7 +6923,7 @@
 		break;
 	case i_SWAP:
 		genamode(curi, curi->smode, "srcreg", sz_long, "src", 1, 0, 0);
-		out("uae_u32 dst = ((src >> 16)&0xFFFF) | ((src&0xFFFF)<<16);\n");
+		out("uae_u32 dst = (((uae_u32)src >> 16)&0xFFFF) | (((uae_u32)src&0xFFFF)<<16);\n");
 		if (cpu_level >= 2) {
 			genflags(flag_logical, sz_long, "dst", "", "");
 			genastore("dst", curi->smode, "srcreg", sz_long, "src");
@@ -9224,8 +9224,8 @@
 			if (curi->dmode == Dreg) {
 				out("uae_u32 tmp = m68k_dreg(regs, dstreg);\n");
 				out("offset &= 0x1f;\n");
-				out("tmp = (tmp << offset) | (tmp >> (32 - offset));\n");
-				out("bdata[0] = tmp & ((1 << (32 - width)) - 1);\n");
+				out("tmp = (offset > 0 && offset < 32) ? ((tmp << offset) | (tmp >> (32 - offset))) : tmp;\n");
+				out("bdata[0] = tmp & (((uae_u32)1 << (32 - width)) - 1);\n");
 			} else {
 				out("uae_u32 tmp;\n");
 				out("dsta += offset >> 3;\n");
@@ -9251,7 +9251,7 @@
 				out("tmp = 0;\n");
 				break;
 			case i_BFFFO:
-				out("{ uae_u32 mask = 1 << (width - 1);\n");
+				out("{ uae_u32 mask = (uae_u32)1 << (width - 1);\n");
 				out("while (mask) { if (tmp & mask) break; mask >>= 1; offset2++; }}\n");
 				out("m68k_dreg(regs, (extra >> 12) & 7) = offset2;\n");
 				break;
@@ -9261,7 +9261,7 @@
 			case i_BFINS:
 				out("tmp = m68k_dreg(regs, (extra >> 12) & 7);\n");
 				out("tmp = tmp & (0xffffffffu >> (32 - width));\n");
-				out("SET_ALWAYS_NFLG(tmp & (1 << (width - 1)) ? 1 : 0);\n");
+				out("SET_ALWAYS_NFLG(tmp & ((uae_u32)1 << (width - 1)) ? 1 : 0);\n");
 				out("SET_ZFLG(tmp == 0);\n");
 				break;
 			default:
@@ -9273,7 +9273,7 @@
 				|| curi->mnemo == i_BFINS) {
 					if (curi->dmode == Dreg) {
 						out("tmp = bdata[0] | (tmp << (32 - width));\n");
-						out("m68k_dreg(regs, dstreg) = (tmp >> offset) | (tmp << (32 - offset));\n");
+						out("m68k_dreg(regs, dstreg) = (offset > 0 && offset < 32) ? ((tmp >> offset) | (tmp << (32 - offset))) : tmp;\n");
 					} else {
 						out("%s(dsta, bdata, tmp, offset, width);\n", putb);
 					}
diff -ru a/src/cpu/newcpu_common.c b/src/cpu/newcpu_common.c
--- a/src/cpu/newcpu_common.c	2024-05-04 20:10:03
+++ b/src/cpu/newcpu_common.c	2025-01-05 16:50:11
@@ -459,7 +459,7 @@
 	uae_s32 regd = regs.regs[reg];
 	if ((dp & 0x800) == 0)
 		regd = (uae_s32)(uae_s16)regd;
-	regd <<= (dp >> 9) & 3;
+	regd = (uae_s32)((uae_u32)regd << ((dp >> 9) & 3));
 	if (dp & 0x100) {
 
 		uae_s32 outer = 0;
@@ -1266,7 +1266,7 @@
 
 		if (extra & 0x400) {
 			a &= 0xffffffffu;
-			a |= (uae_s64)m68k_dreg (regs, extra & 7) << 32;
+			a |= (uae_u64)m68k_dreg (regs, extra & 7) << 32;
 		}
 
 		if (src == 0) {
@@ -1556,7 +1556,7 @@
 			ps |= ((regs.pipeline_pos & 15) << 16);
 			ps |= ((regs.pipeline_stop & 15) << 20);
 			if (mmu030_opcode == -1)
-				ps |= 1 << 31;
+				ps |= (uae_u32)1 << 31;
 			m68k_areg(regs, 7) -= 4;
 			x_put_long(m68k_areg(regs, 7), ps);
 		}
diff -ru a/src/cpu/readcpu.c b/src/cpu/readcpu.c
--- a/src/cpu/readcpu.c	2022-12-29 16:51:59
+++ b/src/cpu/readcpu.c	2025-01-05 12:23:28
@@ -868,9 +868,11 @@
 	for (srcreg=0; srcreg < sbitdst; srcreg++) {
 		for (dstreg=0; dstreg < dstend; dstreg++) {
 			uae_u16 code = (uae_u16)opcode;
+			uae_u8 spos = (table68k[opcode].spos < 0) ? 0 : table68k[opcode].spos;
+			uae_u8 dpos = (table68k[opcode].dpos < 0) ? 0 : table68k[opcode].dpos;
 
-			code = (code & ~smsk) | (srcreg << table68k[opcode].spos);
-			code = (code & ~dmsk) | (dstreg << table68k[opcode].dpos);
+			code = (code & ~smsk) | (srcreg << spos);
+			code = (code & ~dmsk) | (dstreg << dpos);
 
 			/* Check whether this is in fact the same instruction.
 			* The instructions should never differ, except for the