Re: [hatari-devel] undefined behaviour fixes

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

Am 06.01.2025 um 19:59 schrieb 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.

<cpu_warnings6.txt>

I saw this commit and I am not sure if it should be done this way: https://git.tuxfamily.org/hatari/hatari.git/commit/?id=108c56a1cb71e78326f142f0d892a6cadf5460dc

I think -fwrapv should always be set, because it produces different (safe from undefined behaviour) code. See here for an example: https://stackoverflow.com/questions/47232954/what-does-fwrapv-do

Because I still think it is bad coding style to shift negative values and similar in my opinion we should only set -fwrapv where really necessary. So we should probably only do that in the CPU core, like in my previous patch. As I said previously, the parts of Hatari that are used in Previous including the DSP do not trigger any warning. So Hatari is aside from the CPU core likely to be clean and should stay like that.

Furthermore I think the CPU code should be cleaned from those cases on the long term. So probably do not set -fwrapv if the sanitiser is activated to keep the affected code visible.