Re: [hatari-devel] undefined behaviour fixes

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

> Am 06.01.2025 um 12:47 schrieb Thomas Huth <th.huth@xxxxxxxxx>:
> 
> Am Mon, 6 Jan 2025 12:32:43 +0100
> schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:
> 
>>> Am 06.01.2025 um 12:10 schrieb Thomas Huth <th.huth@xxxxxxxxx>:
> ...
>>> All those other casts for left-shifting signed values look rather
>>> cumbersome, since all modern CPUs nowadays should just get this right
>>> (AFAIK). Maybe it's better to compile with "-fwrapv" and call it a day?
>> 
>> I prefer casting to the correct type because 1U might have different size on different platforms.
> 
> It should not be any problem when the size of int is > 32 bits, and with
> size of ints < 32 bits, the whole code won't work anyway, I guess.
> 
>> I’m not sure we can ignore the shifting and overflow issues. There is no „right“. Different platforms/CPUs might behave differently. I already run into such an issue when running Previous on ARM for the first time. There was a very hard to debug difference in handling overflow while casting from float to int.
> 
> Yes, but this is about representation of negative integer numbers, and
> AFAIK all modern CPUs use two's complement in that case. So you're trying
> to fix the code here for CPUs that are not in use anymore since > 40 years,
> I guess.
> 
> See also:  https://stackoverflow.com/a/3789752
> 
> I'd suggest that you try to add "-fwrapv" to the compiler flags and have a
> look which warnings are still left there without your patch.
> 
> Thomas


Using -fwrapv flag with my compiler reduces the amount of warnings. 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");
 				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