Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glebm
Copy link
Contributor

@glebm glebm commented Jan 29, 2021

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)

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.
@hrydgard
Copy link
Owner

Curious, what environment has -O3 as default Release flags? Seems very ill-advised.

@glebm
Copy link
Contributor Author

glebm commented Jan 29, 2021

CMake itself has O3 as the default for Release:

https://gitlab.kitware.com/cmake/cmake/-/blob/c5691f03e5626f30c432d325ab75117ad1e98bd8/Modules/Compiler/GNU.cmake#L58

Many embedded distributions, including batocera.linux, use O3.

@hrydgard
Copy link
Owner

Huh, I wasn't aware of that. I like to stick to -O2.

Would be interesting if someone could bisect that -O3 failure though..

@glebm
Copy link
Contributor Author

glebm commented Jan 29, 2021

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.

@glebm
Copy link
Contributor Author

glebm commented Jan 29, 2021

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

@glebm glebm marked this pull request as ready for review January 29, 2021 22:06
@glebm
Copy link
Contributor Author

glebm commented Jan 29, 2021

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

@hrydgard hrydgard added this to the v1.12.0 milestone Jan 29, 2021
@glebm
Copy link
Contributor Author

glebm commented Jan 29, 2021

Turns out I was not compiling with UBSAN because this project overrides the default CMake configuration to Release

@orbea
Copy link
Contributor

orbea commented Feb 14, 2021

Using -O3 is a good way to introduce experimental compiler features which may cause issues. I agree with this change, but it would be good if the cmake default was changed to -O2 when the user does not set the flags themselves.

@glebm
Copy link
Contributor Author

glebm commented Feb 14, 2021

Using -O3 is a good way to introduce experimental compiler features which may cause issues.

This hasn't been true for over a decade (perhaps 2)

@orbea
Copy link
Contributor

orbea commented Feb 14, 2021

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.

@glebm
Copy link
Contributor Author

glebm commented Feb 14, 2021

There is a good explanation here: https://stackoverflow.com/a/11546263/181228
-O3 is intended to be safe in both gcc and clang. No unsafe optimizations are enabled by it, ever. The flag that enables unsafe optimizations is -Ofast.

CMake defaults depend on the compiler.

@orbea
Copy link
Contributor

orbea commented Feb 14, 2021

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?

As a personal note, I am running production software in the financial sector for many years now with -O3 and have not yet encountered a bug that would not have been there if I would have used -O2.

libretro/bsnes-mercury#47
libretro/bsnes-mercury#50

@unknownbrackets
Copy link
Collaborator

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]

@orbea
Copy link
Contributor

orbea commented Feb 14, 2021

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.

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.". :)

@glebm
Copy link
Contributor Author

glebm commented Mar 3, 2021

All of the UBSAN issues have been fixed

@hrydgard hrydgard modified the milestones: v1.12.0, Future-Prio Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants