build: always set -g -O2 in CORE_CXXFLAGS
#29205
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
theuni
left a comment
theuni
left a comment
There was a problem hiding this comment.
Concept ACK.
My only hesitation here was: can the user revert back to the default (no flag) behavior if desired. The gcc manpages answer this explicitly:
Level 0 produces no debug information at all. Thus, -g0 negates -g
and
-O0 Reduce compilation time and make debugging produce the expected results. This is the default.
So if the user uses: ./configure CXXFLAGS="-O0 -g0", the build should end up using:
-g -O2 ... -O0 -g0, which gets us back to the same behavior as if no flags were used at all.
|
Concept ACK |
fanquake
force-pushed
the
core_cxxflags_always_O2
branch
from
8d8855b to
e86542e
Compare
This avoids cases of missing -O2, when *FLAGS has been overriden. Removes the need for duplicate code to clear autoconf defaults. Also, move CORE_CXXFLAGS before DEBUG_CXXFLAGS, so that -O2 is always overriden if debugging etc.
This was being used to avoid a missing -O2. After the previous commits, this is no-longer an issue.
-O0 is just overriding -Og.
fanquake
force-pushed
the
core_cxxflags_always_O2
branch
from
e86542e to
00c1e2a
Compare
|
Guix builds (x86): |
|
Why not change the behavior around the EDIT: Looked this over a bit more and doesn't seem to be worth it. The |
|
While always overriding Autotools' defaults, are there any particular reasons not using |
AFAIK |
How? FWIW, |
I remember these discussions almost 10 years ago when compilers (notably gcc) put aggressive and (iirc?) non-standards-compliant optimizations behind Either way, it's out of scope for this PR :) |
theuni
left a comment
theuni
left a comment
There was a problem hiding this comment.
ACK 00c1e2a
Quickly tested with macos + depends, which I consider to be one of our wonkiest builds wrt flags. Looks correct to me with or without overridden CXXFLAGS.
I also think having CORE_CXXFLAGS come first generally makes more sense so that it's always override-able.
6 participants
Rather than trying to sporadically rely on / override Autoconf default behaviour. Just always override (if unset), and always set the flags we want (which are the same as the Autoconf defaults).
Removes the need for duplicate code to clear (if not overridden)
CXXFLAGS.Fixes cases of "missing"
-O2. i.e this PR when running a Valgrind CI job with changes here:CXXFLAGS = -g -O2 -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -mbranch-protection=bti -Werror -fsanitize=fuzzer -gdwarf-4Fixes configure output to reflect actual compilation flag ordering, so it's useful.
Note that if we do still end up with a duplicate "-g -O2" when compiling, that has no effect, and I don't really thinks it's something worth trying to optimize.