-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CMakeLists.txt: Stop overriding default cmake cflags #13995
base: master
Are you sure you want to change the base?
Conversation
There doesn't seem to be a specific reason for PPSSPP to override default c(xx)flags, making it difficult for user / build environment to override them.
Curious, what environment has -O3 as default Release flags? Seems very ill-advised. |
CMake itself has O3 as the default for Release: Many embedded distributions, including batocera.linux, use O3. |
Huh, I wasn't aware of that. I like to stick to -O2. Would be interesting if someone could bisect that -O3 failure though.. |
O3 has not been ill-advised since GCC 4 or so (early versions of GCC, e.g. 2.9, were indeed buggy with O3). However, it does require the code to have no undefined behaviour. I ran god of war with UBSAN, didn't see any errors. |
The difference in performance between O2 and O3 can be very significant: https://www.phoronix.com/scan.php?page=article&item=gcc_47_optimizations&num=2 Vectorized code (auto-vectorization is mostly in O3) is even more important for performance on the puny ARM CPUs |
Marking ready for review. I'll make sure the batocera.linux ARM version either does not crash or uses a flag to work around the crash when we update. We're migrating the distro where ppsspp crashed from arm to aarch64 and from gcc-9 to gcc-10 so the issue might solve itself |
Turns out I was not compiling with UBSAN because this project overrides the default CMake configuration to Release |
Using |
This hasn't been true for over a decade (perhaps 2) |
I'm sorry, but you're just wrong. It depends highly on both the code base and gcc (Or clang) version, but yes, its potentially a great way to shoot yourself in the foot. |
There is a good explanation here: https://stackoverflow.com/a/11546263/181228 CMake defaults depend on the compiler. |
Have you ever done the 1000 compile challenge where you first identify the bad optimization, then you identify what file and then what line(s) of code?
|
Compilers have bugs, PSP games have bugs. It's a buggy world out there. We've hit bizarre compiler bugs even on lower optimization levels / debug without any optimizations. I know we do some aliasing and things still that might run against certain optimizations, so I can definitely imagine risks in O3. Doesn't even need to be a compiler bug. That said, I think some things - zlib is a great example, probably UI too - can safely be O3. Even for compiler bugs, it's a bit of a catch-22: the more developers who think O3 is scary, the less who use it and the less who report bugs in it (there's bugs in everything.) Less people reporting bugs = less stable code = self fulfilling prophecy. This goes for PPSSPP too. -[Unknown] |
Good point, but debugging these is often a time consuming and tedious process. Also not everyone is up to the task of providing useful debugging information when everyone else is, "I can't reproduce this.". :) |
All of the UBSAN issues have been fixed |
There doesn't seem to be a specific reason for PPSSPP to override default c(xx)flags, making it difficult for user / build environment to override them.
NOTE: The default Release flags have
-O3
, should be careful with this given #13920 (should probably hold off with this until the underlying bug is fixed)