build: always set `-g -O2` in `CORE

fanquake commented at 4:56 PM on January 8, 2024: member

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-4

Fixes 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.

fanquake requested review from theuni on Jan 8, 2024

fanquake requested review from TheCharlatan on Jan 8, 2024

fanquake requested review from hebasto on Jan 8, 2024

DrahtBot commented at 4:56 PM on January 8, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, hebasto, theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

fanquake added the label DrahtBot Guix build requested on Jan 8, 2024

DrahtBot added the label Build system on Jan 8, 2024

DrahtBot commented at 3:21 AM on January 9, 2024: contributor

Guix builds (on x86_64)

File	commit c2d04f1319a6af6140d17339c4814e0cfc605dd2<br>(master)	commit 4f3bd6ff41e7e33aca3fb951aeac40187574e7f4<br>(master and this pull)
SHA256SUMS.part	`4a9ccfd83cb2679a...`	`e00bd10cf1c552ae...`
*-aarch64-linux-gnu-debug.tar.gz	`4731516310849a25...`	`6b6142d9edfb481f...`
*-aarch64-linux-gnu.tar.gz	`acc376722c5008ac...`	`99182f119ce461e4...`
*-arm-linux-gnueabihf-debug.tar.gz	`27c0ac08bf1e52ec...`	`e8cdae7984b3b71e...`
*-arm-linux-gnueabihf.tar.gz	`f0b4d49bd27eab6f...`	`bce7663712f05abd...`
*-arm64-apple-darwin-unsigned.tar.gz	`aa5defd0392ff711...`	`6260b903586f1099...`
*-arm64-apple-darwin-unsigned.zip	`1f3782346e0788fb...`	`94bfbad8c61f8ed6...`
*-arm64-apple-darwin.tar.gz	`9158f185342766ec...`	`34b751095f3789f9...`
*-powerpc64-linux-gnu-debug.tar.gz	`d319c0b5068715a4...`	`d1631b63f16d6781...`
*-powerpc64-linux-gnu.tar.gz	`cf5f1ca24dc9b0ea...`	`ee9238587275333d...`
*-powerpc64le-linux-gnu-debug.tar.gz	`973602effdc471c7...`	`c0c69fd5d9306991...`
*-powerpc64le-linux-gnu.tar.gz	`1ac53be19e8814c5...`	`9e4e26603055fd46...`
*-riscv64-linux-gnu-debug.tar.gz	`b4ca3e5fd9e64c1f...`	`1cb05ed329bece2d...`
*-riscv64-linux-gnu.tar.gz	`c9ac1a5744a91f9d...`	`0c0706cef8a27838...`
*-x86_64-apple-darwin-unsigned.tar.gz	`a73b36e47baa7d35...`	`ece0b6e6813d368a...`
*-x86_64-apple-darwin-unsigned.zip	`c4ef03f6f0f9e966...`	`fac30c20be157400...`
*-x86_64-apple-darwin.tar.gz	`a02aa184f82f0486...`	`fd58d8f1fafc9079...`
*-x86_64-linux-gnu-debug.tar.gz	`69ab336e5f461987...`	`6d2faee9f85cc86a...`
*-x86_64-linux-gnu.tar.gz	`93f83237256d1b88...`	`c5282beaad43e3b0...`
*.tar.gz	`c8c797f1406aceb4...`	`e6a1b1736dbe879d...`
guix_build.log	`e74fb88f9dbe8f8e...`	`da7970b296d8170e...`
guix_build.log.diff		`5047bfe3bc2dfb58...`

DrahtBot removed the label DrahtBot Guix build requested on Jan 9, 2024

in configure.ac:875 in 8d8855b3f3 outdated

 876 | -  dnl them, to prevent autoconfs "-g -O2" being added. Otherwise we'd end up
 877 | -  dnl with "--coverage -Og -O0 -g -O2".
 878 | -  if test "$CXXFLAGS_overridden" = "no"; then
 879 | -  CXXFLAGS=""
 880 | -  fi
 881 |    CORE_CXXFLAGS="$CORE_CXXFLAGS -Og -O0"

theuni commented at 10:55 AM on January 9, 2024:

Unrelated to this PR, but this line is busted. gcc/clang take the last -O option, so -O0 is just clobbering the -Og here.

TheCharlatan commented at 1:27 PM on January 9, 2024:

Might have been a typo?

fanquake commented at 1:30 PM on January 9, 2024:

@theuni If you want I can also just include a fix for this in here, while we are sorting out flags.

TheCharlatan commented at 1:14 PM on January 10, 2024:

Would be good to fix this here imo.

fanquake commented at 1:58 PM on January 10, 2024:

I've pushed up a commit to do so.

theuni commented at 2:44 PM on January 10, 2024:

Looks good, thanks. Looking at 7b00595d335915dc2bf856e3569115996381a402 it's clear that this is what was intended.

theuni commented at 11:01 AM on January 9, 2024: member

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.

TheCharlatan commented at 1:20 PM on January 9, 2024: contributor

Concept ACK

fanquake force-pushed on Jan 10, 2024

DrahtBot added the label CI failed on Jan 15, 2024

build: always set -g -O2 in CORE_CXXFLAGS

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.

08cd5aca18

build: add sanitizer flags to configure output 6cc2a38c13

ci: cleanup C*FLAG usage in Valgrind jobs

This was being used to avoid a missing -O2. After the previous commits,
this is no-longer an issue.

1dc2c9b385

build: fix optimisation flags used for --coverage

-O0 is just overriding -Og.

00c1e2aa44

fanquake force-pushed on Jan 16, 2024

DrahtBot removed the label CI failed on Jan 16, 2024

TheCharlatan commented at 4:30 PM on January 17, 2024: contributor

Guix builds (x86):

c65c40452ea0c2485c8a17a4d79f4befd2f3f5b66af5281a7833842990a236be  guix-build-00c1e2aa4496/output/aarch64-linux-gnu/SHA256SUMS.part
c027a5f50f2b767a251953eded0cef07adf932f4905e5bd6cb1b19fd013a9f8d  guix-build-00c1e2aa4496/output/aarch64-linux-gnu/bitcoin-00c1e2aa4496-aarch64-linux-gnu-debug.tar.gz
2baa2e82b8af6171b7861e6487fb3c1a07f0b0637e1518fb047d9f0773922781  guix-build-00c1e2aa4496/output/aarch64-linux-gnu/bitcoin-00c1e2aa4496-aarch64-linux-gnu.tar.gz
7452fa6211244d1960795315a2a7292bb9c5ebb5510bfd18fea4f143dfe6f9a1  guix-build-00c1e2aa4496/output/arm-linux-gnueabihf/SHA256SUMS.part
a5160df4bcb278e0df00b2f0b5bfeacf3ab6621bab2e1e4a2dc9e49e8f04a9af  guix-build-00c1e2aa4496/output/arm-linux-gnueabihf/bitcoin-00c1e2aa4496-arm-linux-gnueabihf-debug.tar.gz
b5e7583cf0ac8915971d60abb60395f9edfd3879bf6e15464fc26bbb785e0f2a  guix-build-00c1e2aa4496/output/arm-linux-gnueabihf/bitcoin-00c1e2aa4496-arm-linux-gnueabihf.tar.gz
d89a4870514f1303c5411a4df273ebef942b6f5ab4d04b6f483c47a50ce4356f  guix-build-00c1e2aa4496/output/arm64-apple-darwin/SHA256SUMS.part
b1db6d50a08c8b96752a17cd12310d9103820cbb9edcfb24a4a103115c836c97  guix-build-00c1e2aa4496/output/arm64-apple-darwin/bitcoin-00c1e2aa4496-arm64-apple-darwin-unsigned.tar.gz
16c5657afd564f35cb10f3984ffe50a5235e322a7414a032f94a9de3121ec646  guix-build-00c1e2aa4496/output/arm64-apple-darwin/bitcoin-00c1e2aa4496-arm64-apple-darwin-unsigned.zip
81da4127cda60c8ceb72f99500e92e065c7390ab5803f67a2c3b707c146f5d36  guix-build-00c1e2aa4496/output/arm64-apple-darwin/bitcoin-00c1e2aa4496-arm64-apple-darwin.tar.gz
541ae4853167d1a7928f4bb52903dac4a5b6da439af27077db17a3c4e980b2e9  guix-build-00c1e2aa4496/output/dist-archive/bitcoin-00c1e2aa4496.tar.gz
f9499295b61858074a93e918b857d8c77b645f126b1c9ead4d20a3ca8fed3d3c  guix-build-00c1e2aa4496/output/powerpc64-linux-gnu/SHA256SUMS.part
3ef20afaa28b4382755e00f2f01d8b933e8c602c2bc0a0a50f5ad9a7c1b19012  guix-build-00c1e2aa4496/output/powerpc64-linux-gnu/bitcoin-00c1e2aa4496-powerpc64-linux-gnu-debug.tar.gz
e5caf85e5c02fbe2c21a65478155e225c0be07c3eec70862e9ba3578dfbb02a0  guix-build-00c1e2aa4496/output/powerpc64-linux-gnu/bitcoin-00c1e2aa4496-powerpc64-linux-gnu.tar.gz
abc4042178ac514479f1340b07f5c92fd557e9a5c2dc71f0fe8086d9bfecb233  guix-build-00c1e2aa4496/output/powerpc64le-linux-gnu/SHA256SUMS.part
2b0283f238e11025723d6f1a0d1af3fe3069d35d0968bfef9633cf5bd6dc24b0  guix-build-00c1e2aa4496/output/powerpc64le-linux-gnu/bitcoin-00c1e2aa4496-powerpc64le-linux-gnu-debug.tar.gz
14ce3c4ec2c5f6310faf9e700af667a1ea3a9ecdc5f3633a8fba5865cc32ce7f  guix-build-00c1e2aa4496/output/powerpc64le-linux-gnu/bitcoin-00c1e2aa4496-powerpc64le-linux-gnu.tar.gz
89b139b4c19a817c6bbf307692ebfbcbe558bae3146b99e524a11f5a6750c152  guix-build-00c1e2aa4496/output/riscv64-linux-gnu/SHA256SUMS.part
1b9f0041dea802693cf31198b06466841b036c0be358f9d3f8002ebf7468c18d  guix-build-00c1e2aa4496/output/riscv64-linux-gnu/bitcoin-00c1e2aa4496-riscv64-linux-gnu-debug.tar.gz
149438cc56f310448d9ee3e468352fae707080aeb5dbd36cfea24e7297866fdf  guix-build-00c1e2aa4496/output/riscv64-linux-gnu/bitcoin-00c1e2aa4496-riscv64-linux-gnu.tar.gz
321aadc287d74256d082379efd1061965293275ab683ae21a67d33a1f4975167  guix-build-00c1e2aa4496/output/x86_64-apple-darwin/SHA256SUMS.part
4e9f37bcfbe109392748473c6081fe2842e9c34aac0430dd8f34b17b5ea97e14  guix-build-00c1e2aa4496/output/x86_64-apple-darwin/bitcoin-00c1e2aa4496-x86_64-apple-darwin-unsigned.tar.gz
cc2a2e18ca8a9027d64269c2a4149b197d57147e607373cf410798a20b7b8b82  guix-build-00c1e2aa4496/output/x86_64-apple-darwin/bitcoin-00c1e2aa4496-x86_64-apple-darwin-unsigned.zip
06b945c60e51d89688bab5b8268ca763a094be75f9ee5927ed5c6c85d0d73e3c  guix-build-00c1e2aa4496/output/x86_64-apple-darwin/bitcoin-00c1e2aa4496-x86_64-apple-darwin.tar.gz
576f8bf1f3d554ae1e5c7a87c065fac80b703601bf7b02893fc47d26d5b8009c  guix-build-00c1e2aa4496/output/x86_64-linux-gnu/SHA256SUMS.part
adc8a1e5eda8b456cc1e6288468a732dc02724579b35c53869fbb028071ae839  guix-build-00c1e2aa4496/output/x86_64-linux-gnu/bitcoin-00c1e2aa4496-x86_64-linux-gnu-debug.tar.gz
e64f41c4bd2f5e364f59ef9540a3087bc3ad4f9c8699636b95615a53e8d5462b  guix-build-00c1e2aa4496/output/x86_64-linux-gnu/bitcoin-00c1e2aa4496-x86_64-linux-gnu.tar.gz
410c0435aa0d1df59e0ed829a8887edddc07802c3a2960588d02ddb153275f68  guix-build-00c1e2aa4496/output/x86_64-w64-mingw32/SHA256SUMS.part
ab4e1f86f21e945e8cafaa4b1f69dbde44abead331dd54df4212b460fa15ca3b  guix-build-00c1e2aa4496/output/x86_64-w64-mingw32/bitcoin-00c1e2aa4496-win64-debug.zip
9439627944b4865a891779892710d1bde4327a660837c2bd4d8251654038b4e4  guix-build-00c1e2aa4496/output/x86_64-w64-mingw32/bitcoin-00c1e2aa4496-win64-setup-unsigned.exe
cd3b8e43d2e9c57e0894a8b5a40696be3cfcf9462682a7aebd58d8889556d216  guix-build-00c1e2aa4496/output/x86_64-w64-mingw32/bitcoin-00c1e2aa4496-win64-unsigned.tar.gz
2b3bb6f1b4edcbdf6b798f976b1ef388b9dda86675d9b1fb5adbdb07d9b1dee7  guix-build-00c1e2aa4496/output/x86_64-w64-mingw32/bitcoin-00c1e2aa4496-win64.zip

TheCharlatan commented at 9:30 PM on January 17, 2024: contributor

Why not change the behavior around the CFLAGS as well? That seems a bit confusing to me; we now have one variable that always retains -g -O2 when overriding and another that does not.

EDIT: Looked this over a bit more and doesn't seem to be worth it. The CFLAGS are hardly used, and where they are used in secp, they get applied in essentially the same manner as here.

TheCharlatan approved

TheCharlatan commented at 8:20 AM on January 18, 2024: contributor

lgtm ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533

DrahtBot requested review from theuni on Jan 18, 2024

hebasto commented at 5:46 PM on January 22, 2024: member

While always overriding Autotools' defaults, are there any particular reasons not using -O3 instead of -O2?

hebasto approved

hebasto commented at 6:02 PM on January 22, 2024: member

ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533, I have reviewed the code and it looks OK. Also tested ci/test/00_setup_env_native_valgrind.sh.

luke-jr commented at 3:41 AM on January 23, 2024: member

While always overriding Autotools' defaults, are there any particular reasons not using -O3 instead of -O2?

AFAIK -O3 isn't safe.

hebasto commented at 1:40 PM on January 23, 2024: member

While always overriding Autotools' defaults, are there any particular reasons not using -O3 instead of -O2?

AFAIK -O3 isn't safe.

How?

FWIW, -O3 is used by default in CMake for "Release" builds.

theuni commented at 3:52 PM on January 24, 2024: member

While always overriding Autotools' defaults, are there any particular reasons not using -O3 instead of -O2?

AFAIK -O3 isn't safe.

I remember these discussions almost 10 years ago when compilers (notably gcc) put aggressive and (iirc?) non-standards-compliant optimizations behind -O3. I'm guessing that's probably not the case anymore these days, but I do agree somewhat with @luke-jr that we'd need to investigate the safety of this before blindly changing.

Either way, it's out of scope for this PR :)

theuni approved

theuni commented at 4:13 PM on January 24, 2024: member

ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533

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.

fanquake merged this on Jan 25, 2024

fanquake closed this on Jan 25, 2024

fanquake deleted the branch on Jan 25, 2024

bitcoin locked this on Jan 24, 2025

build: always set `-g -O2` in `CORE_CXXFLAGS` #29205

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)