Re: [hatari-devel] undefined behaviour fixes

[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.

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-04 18:02:56
@@ -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-05 12:17:37
@@ -3441,20 +3441,20 @@
 			case sz_byte:
 #ifdef USE_DUBIOUS_BIGENDIAN_OPTIMIZATION
 				/* This causes the target compiler to generate better code on few systems */
-				out("uae_s8 %s = ((uae_u8*)&m68k_dreg(regs, %s))[3];\n", name, reg);
+				out("uae_u8 %s = ((uae_u8*)&m68k_dreg(regs, %s))[3];\n", name, reg);
 #else
-				out("uae_s8 %s = m68k_dreg(regs, %s);\n", name, reg);
+				out("uae_u8 %s = m68k_dreg(regs, %s);\n", name, reg);
 #endif
 				break;
 			case sz_word:
 #ifdef USE_DUBIOUS_BIGENDIAN_OPTIMIZATION
-				out("uae_s16 %s = ((uae_s16*)&m68k_dreg(regs, %s))[1];\n", name, reg);
+				out("uae_u16 %s = ((uae_s16*)&m68k_dreg(regs, %s))[1];\n", name, reg);
 #else
-				out("uae_s16 %s = m68k_dreg(regs, %s);\n", name, reg);
+				out("uae_u16 %s = m68k_dreg(regs, %s);\n", name, reg);
 #endif
 				break;
 			case sz_long:
-				out("uae_s32 %s = m68k_dreg(regs, %s);\n", name, reg);
+				out("uae_u32 %s = m68k_dreg(regs, %s);\n", name, reg);
 				break;
 			default:
 				term();
@@ -3469,10 +3469,10 @@
 		if (getv == 1)
 			switch (size) {
 			case sz_word:
-				out("uae_s16 %s = m68k_areg(regs, %s);\n", name, reg);
+				out("uae_u16 %s = m68k_areg(regs, %s);\n", name, reg);
 				break;
 			case sz_long:
-				out("uae_s32 %s = m68k_areg(regs, %s);\n", name, reg);
+				out("uae_u32 %s = m68k_areg(regs, %s);\n", name, reg);
 				break;
 			default:
 				term();
@@ -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) {
@@ -8360,7 +8360,7 @@
 		if (cpu_level <= 1) {
 			addcycles000_nonces("getDivs68kCycles((uae_s32)dst, (uae_s16)src, &extra)");
 		}
-		out("if (dst == 0x80000000 && src == -1) {\n");
+		out("if (dst == 0x80000000 && (uae_s16)src == -1) {\n");
 		out("setdivsflags((uae_s32)dst, (uae_s16)src);\n");
 		out("} else {\n");
 		out("uae_s32 newv = (uae_s32)dst / (uae_s32)(uae_s16)src;\n");
@@ -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-01 14:31:05
@@ -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