build: Add --with-append-cxxflags option

MarcoFalke commented at 8:00 am on June 5, 2021: member

This can be useful for testing. For example #22064 (comment) could use --with-append-cxxflags="-ftrivial-auto-var-init=pattern" without having to specify the default arguments again (-g -O2).

MarcoFalke added the label Needs Guix build on Jun 5, 2021

fanquake added the label Build system on Jun 5, 2021

in configure.ac:1454 in faf33ed78a outdated

1449@@ -1444,6 +1450,10 @@ fi
1450 
1451 AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "x$use_external_signer" = "xyes"])
1452 
1453+if test x$use_trivial_auto_var_init_pattern = xyes; then
1454+  AX_CHECK_COMPILE_FLAG([-ftrivial-auto-var-init=pattern],[CXXFLAGS="$CXXFLAGS -ftrivial-auto-var-init=pattern"],,[[$CXXFLAG_WERROR]])

fanquake commented at 8:22 am on June 5, 2021:

0if test x$use_trivial_auto_var_init_pattern != xno; then
1  AX_CHECK_COMPILE_FLAG([-ftrivial-auto-var-init=$use_trivial_auto_var_init_pattern],[CXXFLAGS="$CXXFLAGS -ftrivial-auto-var-init=$use_trivial_auto_var_init_pattern"],,[[$CXXFLAG_WERROR]])

MarcoFalke commented at 8:43 am on June 5, 2021:

I avoided that because the only options are uninitialized (already default), zero (needs -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang), pattern (can be enabled with the new boolean option).

Happy to apply your patch, but I think it also needs -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang, which again will throw a warning if used with pattern. As this is only used for testing, a warning is fine by me.

fanquake commented at 8:24 am on June 5, 2021: member

Related discussion in #18892.

Note that -ftrivial-auto-var-init=zero seems to be going away. If I use that with (Apple) Clang I see:

0clang++ a.cpp -std=c++17 -ftrivial-auto-var-init=zero      
1clang: error: -ftrivial-auto-var-init=zero hasn't been enabled. Enable it at your own peril for benchmarking purpose only with -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang

Tested this PR + my change, using -ftrivial-auto-var-init=uninitialized.

practicalswift commented at 4:59 pm on June 5, 2021: contributor

Concept ACK

Very nice to see! Let’s kill the use of uninitialized memory (UUM) bug class! :)

DrahtBot commented at 6:27 pm on June 5, 2021: member

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

Conflicts

No conflicts as of last run.

MarcoFalke removed the label Needs Guix build on Jun 5, 2021

laanwj commented at 2:12 pm on June 7, 2021: member

I’m honestly not sure why this warrants adding another build system configuration flag. For new people buildng from source having so many options only vaguely related to bitcoin’s application is confusing. Why not pass it in through *FLAGS when needed?

Alternatively maybe we can group these kind of “warning: testing only” options together and have a single option. But I don’t like having a separate one for it.

MarcoFalke commented at 3:22 pm on June 7, 2021: member

I agree that it is stupid to wrap each compiler option into a config option. While using CXXFLAGS is certainly possible, it is not straightforward, because it comes with some unexpected downsides, like disabling -g.

Compare the default CXXFLAGS:

0  CXXFLAGS        =  -fdebug-prefix-map=$(abs_srcdir)=.  -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection  -Wall -Wextra -Wformat -Wformat-security -Wvla -Wswitch -Wredundant-decls -Wunused-variable -Wdate-time -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override  -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-deprecated-copy   -g -O2 -fno-extended-identifiers

CXXFLAGS with “pattern”:

0  CXXFLAGS        =  -fdebug-prefix-map=$(abs_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection     -ftrivial-auto-var-init=pattern

fanquake commented at 3:30 am on June 9, 2021: member

I slightly misunderstood the intent when first reviewing, and agree with @laanwj here. Adding a new configuration option just to turn on a single compiler option is overkill. If it’s not possible to achieve what you’d like, without downsides, when using CXXFLAGS, that seems like something we need to fix.

laanwj commented at 6:48 am on June 9, 2021: member

If it’s not possible to achieve what you’d like, without downsides, when using CXXFLAGS, that seems like something we need to fix.

Agree, if CXXFLAGS as a mechanism to pass arbitrary options in is broken, that’s what we need to fix. Alternatively, if that is not possible, to add a method to pass arbitrary options that actually works. Wrapping every option in its own configure option is a no-go.

MarcoFalke force-pushed on Jun 10, 2021

MarcoFalke renamed this:
~~build: Add --enable-trivial-auto-var-init-pattern option~~
build: Add --with-append-cxxflags option
on Jun 10, 2021

MarcoFalke commented at 9:24 am on June 10, 2021: member

Tried to addressed feedback

luke-jr referenced this in commit 0201160736 on Jun 27, 2021

build: Add --with-append-cxxflags option fa14c6818f

MarcoFalke force-pushed on Jul 7, 2021

luke-jr approved

luke-jr commented at 9:23 pm on July 8, 2021: member

utACK

Seems the motivation is to add CXXFLAGS without losing whatever default flags autotools gave us previously. I probably won’t use it, but it seems harmless and possibly useful to others, so why not?

practicalswift commented at 10:09 pm on July 8, 2021: contributor

cr ACK fa14c6818f4094669584a110a517fa1347f1f36e

Nice not having to repeat the defaults.

laanwj commented at 3:05 pm on August 18, 2021: member

But if we start doing this for cxxflags, where does it stop. I still disagree with this, it’s a non-standard convention, It saves so little effort. But whatever if everyone else thinks it is a good idea.

MarcoFalke commented at 5:20 pm on August 18, 2021: member

Yeah, it would be nice if build systems were a bit more user friendly.

I guess I’ll just keep typing “-g -O2” every time.

MarcoFalke closed this on Aug 18, 2021

MarcoFalke deleted the branch on Aug 18, 2021

luke-jr referenced this in commit ac07122c1a on May 21, 2022

DrahtBot locked this on Aug 18, 2022

build: Add –with-append-cxxflags option #22159

Conflicts