search

LIJI32 / SameBoy

Game Boy and Game Boy Color emulator written in C
https://sameboy.github.io/
Other
1.58k stars 206 forks source link

apu: Do not dereference type-punned pointers #593

Closed carmiker closed 3 months ago

carmiker commented 4 months ago

Dereferencing type-punned pointers will break strict-aliasing rules.

orbea commented 4 months ago

@LIJI32

This was revealed by a Gentoo tinderbox reported for the bsnes-jg package.

https://bugs.gentoo.org/926077

When asking in their IRC channel I was told that strict-aliasing violations are cases where LTO can assume its allowed to optimize impossible code that potentially achieves entirely incorrect results. Their standard fix is to restrict LTO and use -fno-strict-aliasing, but it seems better to just fix the warnings.

thesamesam commented 4 months ago

It can go wrong anyway, it's just more likely with LTO. We try to report things upstream if upstream is alive before restricting LTO (or do it together with a link to the upstream report so we can undo it when fixed). A lot of things have got fixed that way :)

eli-schwartz commented 4 months ago

When asking in their IRC channel I was told that strict-aliasing violations are cases where LTO can assume its allowed to optimize impossible code that potentially achieves entirely incorrect results. Their standard fix is to restrict LTO and use -fno-strict-aliasing, but it seems better to just fix the warnings.

It's not the standard fix, it's the standard workaround.

The difference is subtle. Restricting LTO and using -fno-strict-aliasing pessimizes codegen (possibly severely), and is used to prevent immediate breakage in existing versions. It is also standard to report the bug upstream and leave a comment next to the fno-strict-aliasing and restrict-LTO, pointing to the bug report(s) so they can be followed up on and removed once the workaround is no longer necessary (usually after a new upstream release).

My apologies for using wording in IRC that was apparently confusing.

LIJI32 commented 3 months ago

Removing -fno-strict-aliasing worries me a bit, I could be violating strict aliasing rules elsewhere, without getting comipler warnings about them. Also, I'm not why mixing -fno-strict-aliasing with LTO is a problem?

thesamesam commented 3 months ago

The patch is removing -Wno-strict-aliasing, not -fno-strict-aliasing (which doesn't appear to be being set at all).

Strict aliasing violations are bad in general, but with LTO, it's more likely that the compiler will take advantage of aliasing assumptions. It also exposes more opportunities for that to happen because of cross-TU inlining.

orbea commented 3 months ago

Removing -fno-strict-aliasing worries me a bit, I could be violating strict aliasing rules elsewhere, without getting comipler warnings about them.

They are enabled with -Wall unless -Wno-strict-aliasing is used.

LIJI32 commented 3 months ago

Right, for some reason I thought I had -fno-strict-aliasing as well. 🤦‍♂️ Alright then, I did a few tests and it seems that a union results in slightly better codegen, so I'll be using that.