Re: [hatari-devel] undefined behaviour fixes

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

> Am 06.01.2025 um 19:27 schrieb Thomas Huth <th.huth@xxxxxxxxx>:
> 
> Am Mon, 6 Jan 2025 14:11:05 +0100
> schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:
> ...
>> Using -fwrapv flag with my compiler reduces the amount of warnings.
> 
> Thanks for checking - that patch looks more reasonable to me now.
> 
>> There are still some warnings about shifting beyond type size (uae_u32 x
>>>> 32 and uae_u32 x << 32) and shifting with negative value (x << -1).
>> Appended patch should fix them.
> 
> 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 14:04:13
> @@ -9224,7 +9224,7 @@
> 			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("tmp = (offset > 0 && offset < 32) ? ((tmp << offset) | (tmp >> (32 - offset))) : tmp;\n"); 
> 
> offset is restricted to 0x1f, so it is always < 32, i.e. it should be
> sufficient to check for "offset != 0" in above condition. I'd maybe also
> rather write it like this:
> 
> out("tmp = (tmp << offset)\n");
> out("if (offset) tmp |= (tmp >> (32 - offset));\n");
> 
> ?
> 
> Thomas
> 

I agree. I appended a new patch, also including the -fwrapv flag.

diff -ru a/src/cpu/CMakeLists.txt b/src/cpu/CMakeLists.txt
--- a/src/cpu/CMakeLists.txt	2024-05-04 20:10:03
+++ b/src/cpu/CMakeLists.txt	2025-01-06 16:15:32
@@ -56,6 +56,7 @@
 	# Silence some warnings for the autogenerated sources - the generated
 	# cpuemu_xx.c contains a lot of warnings we don't really care about...
 	set(CPUEMU_CFLAGS "-Wno-sign-compare -Wno-unused-variable")
+	set(CPUEMU_CFLAGS "${CPUEMU_CFLAGS} -fwrapv")
 	if (WARN_SHADOW_LOCAL_AVAILABLE)
 		set(CPUEMU_CFLAGS "${CPUEMU_CFLAGS} -Wno-shadow=local")
 	endif()
@@ -70,6 +71,7 @@
 	# try to keep the files in sync with WinUAE, it's hard to fix them)
 	set(CPUMAIN_CFLAGS "-Wno-unused-variable -Wno-unused-function -Wno-unused-label")
 	set(CPUMAIN_CFLAGS "${CPUMAIN_CFLAGS} -Wno-missing-braces -Wno-sign-compare")
+	set(CPUMAIN_CFLAGS "${CPUMAIN_CFLAGS} -fwrapv")
 	if (WARN_FALLTRHOUGH_AVAILABLE)
 		set(CPUMAIN_CFLAGS "${CPUMAIN_CFLAGS} -Wno-implicit-fallthrough")
 	endif()
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 19:55:16
@@ -9224,7 +9224,7 @@
 			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("if(offset) tmp = (tmp << offset) | (tmp >> (32 - offset));\n");
 				out("bdata[0] = tmp & ((1 << (32 - width)) - 1);\n");
 			} else {
 				out("uae_u32 tmp;\n");
@@ -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/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-06 14:04:13
@@ -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