tonioni / WinUAE

WinUAE Amiga emulator
http://www.winuae.net/
528 stars 86 forks source link

Undefined behavior in setchkundefinedflags and setchk2undefinedflags #228

Open dirkwhoffmann opened 1 year ago

dirkwhoffmann commented 1 year ago

Hi Toni,

I am referring to functions setchkundefinedflags and setchk2undefinedflags in newcpu_common.cpp.

Inside both functions, multiple subtractions are performed such as this one:

if (val > lower && upper - val >= 0)

When running real test cases (e.g., the ones created by cputester), integer underflows may occur which is undefined behavior in C. This is not a problem for MSVC (I think), but it is in clang.

I’ve noticed the issue after porting this code to vAmiga and running cputester. The result was:

Thing is that clang sometimes uses aggressive optimizations that exploit undefined behavior.

In my code, I’ve fixed the issue by replacing the line above by

if (val > lower && diff(upper,val) >= 0)

where diff is defined as

           auto diff = [&](i32 arg1, i32 arg2) {
                return i32((i64)arg1 - (i64)arg2);
            };

As stated above, I think it’s not an issue with MSVC, but if you wish to make your code more robust, you might want to change the troublesome lines by something like:

if (… && (uae_s32)((uae_s64)upper - (uae_s64)val) >= 0)
tonioni commented 1 year ago

Looks a bit too ugly, hopefully there is some better way to fix this cleanly.