Fix issues found by UBSan and ASan#1842
Fix issues found by UBSan and ASan#1842fabioarnold wants to merge 8 commits intocemu-project:mainfrom
Conversation
| { | ||
| return _swapEndianU32(*(uint32*)(memory_base + address)); | ||
| uint32 v; | ||
| memcpy(&v, memory_base + address, sizeof(uint32)); |
There was a problem hiding this comment.
Yeah unfortunately we can't actually do this. In debug builds this will compile down to a function call plus some overhead and since the memory_* and ppcMem_* functions are all critical for performance (literally called many millions of times per frame) it would hurt performance too much. I further suspect that in release mode it will also not nicely compile down to a single instruction (which the old code does) since the compiler doesn't know that address is guaranteed to be aligned. So it will have to emit a byte-wise copy.
We can probably tell the compiler that the pointer is aligned via some assume construct but I wouldn't trust them to always optimize down to a dword fetch (it certainly wont optimize in debug builds).
C++23 has a solution for this: std::start_lifetime_as. So I think best course of action is to just wait until thats broadly available.
There was a problem hiding this comment.
I observed an unaligned access by the game here. Can't really control or avoid that, right?
I haven't looked at the optimized assembly or measured performance. Would be worth it to check.
There was a problem hiding this comment.
PowerPC has strict alignment requirements so that seems unlikely. Can you paste the call stack?
There was a problem hiding this comment.
This would crash on actual console so there must be a bug somewhere else causing the invalid address. What game is it and when does it trigger?
There was a problem hiding this comment.
Twilight Princess HD. Just loading the game.
There was a problem hiding this comment.
I got rid of the commit that contained memcpy. I can still reliably trigger the unaligned read. This must be the result of another bug.
fabioarnold
force-pushed
the
fabio/fix-ub-issues
branch
from
bd13048 to
3b99ddd
Compare
prevent misaligned pointer use by aligning tightly packed structs to 4
fabioarnold
force-pushed
the
fabio/fix-ub-issues
branch
from
3b99ddd to
6160b95
Compare
wrapping of signed integers is undefined behavior
__builtin_clzl is the 64 bit version For example for _Mask 32 it calculated 31 - 58 = -27 Cast to uint32 gives 4294967269
- bpp=32: blockOutput is texelBaseType* (uint32_t, 4-byte aligned). Casting to uint64* and dereferencing is UB when the address is not 8-byte aligned (e.g. 0x...c is 4-byte aligned but not 8-byte). - bpp=8: blockOutput is texelBaseType* (uint8_t, 1-byte aligned). Casting to uint64* is UB for any address not divisible by 8.
fabioarnold
force-pushed
the
fabio/fix-ub-issues
branch
from
6160b95 to
f26e716
Compare
|
Apart from unaligned reads and writes in PPCInterpreterImpl.cpp with these changes I can load and run TPHD without trapping due to UBSand/ASan now. |
2 participants
These are various issues I encountered while starting up Zelda TPHD in a debug build of Cemu with UBSan and ASan enabled. Each commit fixes one issue.
Most of these are over-/underflows with signed integer arithmetic which is undefined behavior in C++. I fixed this using unsigned ints where over-/underflows are defined as wrapping.