This changes --enable-debug to choose better options:
-Og rather than -O0 if it is available-g3 over -g if it is available$DEBUG_CPPFLAGS rather than updating $CPPFLAGS directlyThis also updates the flags summaries at the end of the configure script to more accurately reflect what flags will be used for most of the code in src.
Good catch! Concept ACK
Just wanted to add that I’ve noticed a lot of high quality contributions from you recently. Really nice to have you on board! Thanks for your contributions!
Concept ACK, but let’s do a bit of cleanup while we’re at it.
I assume these were added before we started properly checking flags. The “GCC” thing probably just dumps “cc –version | grep Free Software Foundation” like lots of other tools.
I’m not sure how portable “-g3” is, and we actually want to be using “-Og” these days instead of “-O0” if possible. We should be checking flags explicitly.
Also, while we’re at it, this is one of the handful of remaining checks that taints the global CPPFLAGS/CXXFLAGS.
0AX_CHECK_COMPILE_FLAG([-Og],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"]],,[[$CXXFLAG_WERROR]])
1AX_CHECK_COMPILE_FLAG([-g3],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -g3"]],,[[$CXXFLAG_WERROR]])
2
3AX_CHECK_PREPROC_FLAG([-DDEBUG],[[DEBUG_CPPFLAGS="$DEBUG_CPPFLAGS -DDEBUG"]],,[[$CXXFLAG_WERROR]])
4AX_CHECK_PREPROC_FLAG([-DDEBUG_LOCKORDER],[[DEBUG_CPPFLAGS="$DEBUG_CPPFLAGS -DDEBUG_LOCKORDER"]],,[[$CXXFLAG_WERROR]])
The CFLAGS line can simply be dropped. (The above doesn’t attempt any fallback in case of failed -Og/-g3, but ideally they would fall back to -O0/-g respectively.)
Then at the end of configure.ac, AC_SUBST(DEBUG_CPPFLAGS) and AC_SUBST(DEBUG_CXXFLAGS), and finally add the respective flags to AM_CXXFLAGS/AM_CPPFLAGS in src/Makefile.am.
Question: do we want to have conditional logic based on GCC vs. Clang, or just do what’s best for GCC (as long as it doesn’t break Clang)? It seems like if we want to have the best symbols for both we need some additional logic here.
GCC:
Clang:
So it seems like “-g -Og” works on both compilers, but for Clang “-g -O0” is actually better.
The $GCC and $GXX variables are set if the compiler defines __GNUC__, which clang does. So those variables should never be used. To actually test for Clang you should check if __clang__ or __llvm__ are defined.
Also FYI later I plan to submit another PR for you to review to clean up the logic we have to start using more of the newer AX_* macros, most notably AX_APPEND_COMPILE_FLAGS which will clean up a lot of copy/paste logic we have when we check a flag and then append it.[1] I’m going to do that as another PR because I want to separate changes that affect logic (like this PR) with changes that are just cleanups.
Summary of recent commits:
606@@ -592,12 +607,15 @@ if test x$use_hardening != xno; then
607 AX_CHECK_COMPILE_FLAG([-Wstack-protector],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -Wstack-protector"])
608 AX_CHECK_COMPILE_FLAG([-fstack-protector-all],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fstack-protector-all"])
609
610- AX_CHECK_PREPROC_FLAG([-D_FORTIFY_SOURCE=2],[
611- AX_CHECK_PREPROC_FLAG([-U_FORTIFY_SOURCE],[
612- HARDENED_CPPFLAGS="$HARDENED_CPPFLAGS -U_FORTIFY_SOURCE"
613+ dnl -D_FORTIFY_SOURCE=1 and 2 incompatible with -O0
Nice investigative work above!
Generally we don’t want to make any assumptions about what compiler will be used, unless an option is known to directly conflict between common ones. I don’t think that’s the case here. The code also builds fine with msvc, and I’d hope that icc works as well.
So checks like this really don’t make much sense because they’re basically just hard-coded:
0if test $CLANG -eq 1; then
1 AX_CHECK_COMPILE_FLAG([-O0],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"]],,[[$CXXFLAG_WERROR]])
2...
Let’s try instead to come up with some universal checks that can be applied.
A few more little things gleaned from glancing at the gcc/clang code:
0 { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fbranch_count_reg, NULL, 1 },
1 { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fmove_loop_invariants, NULL, 1 },
2 { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_ftree_pta, NULL, 1 },
3 { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fssa_phiopt, NULL, 1 },
For the most part, debugging with -O0 is huge overkill due to all of the inlining, so it makes sense to me that gcc/clang chose -O1 as the starting point for debugging.
I think we should use the flags that declare intent, as opposed to trying to guess about optimization levels. That way we benefit more from future compiler versions.
So my preference would be:
We could either nest the fallbacks in the original check’s failure condition, or just set a variable if the original check fails, and try the fallback if it’s false. My slight preference would be for the latter, just because it’s more readable.
Changes here:
There’s one other change here that I would like @luke-jr to review. In 9b4e03b27b04f6dc687409a13859d59bb7909d8f he added logic to first try to undefined the flag before setting it. Since we now pass in the debug/error flags, if the -D_FORTIFY_SOURCE=2 check fails I retry with the undefine change. This should mean that if you use –enable-werror the right thing will happen if just setting -D_FORTIFY_SOURCE=2 warns unless it’s first undefined. I don’t have a compiler that warns in this case, and I’d like Luke to check if he thinks this makes sense.
Also note that the _FORTIFY_SOURCE change is a lot less necessary in the first place now since the original error was related to -O0, and we’re now preferring -Og (and the warning was a GCC thing in the first place). We could just drop the _FORTIFY_SOURCE change altogether since the logic is a bit confusing; that said, the new logic I added seems a little bit better.
600@@ -592,12 +601,16 @@ if test x$use_hardening != xno; then
601 AX_CHECK_COMPILE_FLAG([-Wstack-protector],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -Wstack-protector"])
602 AX_CHECK_COMPILE_FLAG([-fstack-protector-all],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fstack-protector-all"])
603
604- AX_CHECK_PREPROC_FLAG([-D_FORTIFY_SOURCE=2],[
I’d rather not reverse this logic as it now completely depends on -Werror working as intended. If some compiler fails to warn about the redefine, we’d get the wrong behavior here.
With the current logic, we always undefine and redefine as long as the options work individually.
Thanks, looks good to me now other than the FORTIFY reversal.
Also note that the _FORTIFY_SOURCE change is a lot less necessary in the first place now since the original error was related to -O0, and we’re now preferring -Og (and the warning was a GCC thing in the first place). We could just drop the _FORTIFY_SOURCE change altogether since the logic is a bit confusing; that said, the new logic I added seems a little bit better.
Being pedantic: The warning comes from glibc, it could potentially occur with any compiler.
-O0.
One more thing I might as well ask while we’re making this change, since I touched this line of code anyway. At the top of src/Makefile.am we have:
0AM_LDFLAGS = $(PTHREAD_CFLAGS) $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS) $(GPROF_LDFLAGS)
We don’t set LIBTOOL_LDFLAGS anywhere, but we do set LIBTOOL_APP_LDFLAGS in configure.ac and then add to it LDFLAGS all over the place to executables we build. It seems like this was originally a typo and the original intention was to add LIBTOOL_APP_LDFLAGS to AM_LDFLAGS.
The current code works but we might want to fix this (perhaps in another PR).
Hmm it appears the extra flags are supposed to be CPPFLAGS not CFLAGS/CXXFLAGS according to the m4 macro
Not sure what you mean by that. The flags passed in are appended to CPPFLAGS for the test, it doesn’t matter how they’re stored otherwise.
I agree with just leaving it alone, though.
We don’t set LIBTOOL_LDFLAGS anywhere, but we do set LIBTOOL_APP_LDFLAGS in configure.ac and then add to it LDFLAGS all over the place to executables we build. It seems like this was originally a typo and the original intention was to add LIBTOOL_APP_LDFLAGS to AM_LDFLAGS.
The current code works but we might want to fix this (perhaps in another PR).
Yes, it looks like LIBTOOL_LDFLAGS wasn’t removed from AM_LDFLAGS in e0077de5de538dd51b5dbd04e05c998d40b20b30 as it should’ve been.
But the manual additions of LIBTOOL_APP_LDFLAG are intentional; They’re appended for all application links, but not for libraries (libbitcoinconsensus).
1379@@ -1370,9 +1380,9 @@ echo " build os = $BUILD_OS"
1380 echo
1381 echo " CC = $CC"
1382 echo " CFLAGS = $CFLAGS"
1383-echo " CPPFLAGS = $CPPFLAGS"
1384+echo " CPPFLAGS = $CPPFLAGS $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS"
1385 echo " CXX = $CXX"
1386-echo " CXXFLAGS = $CXXFLAGS"
1387-echo " LDFLAGS = $LDFLAGS"
1388+echo " CXXFLAGS = $CXXFLAGS $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS"
Should this line include $(ERROR_CXXFLAGS) and $(GPROF_CXXFLAGS) as well? Maybe this section of configure.ac should include comments to stay in sync with the AM_CXXFLAGS and AM_LDFLAGS lines from Makefile.am.
Could also consider combining the flags into a variable that is used both places.
Updated. What about something like:
0DEFAULT_AM_CXXFLAGS="$CXXFLAGS $DEBUG_CXXFLAGS etc..."
1AC_SUBST(DEFAULT_AM_CXXFLAGS)
2# and likewise for the other vars
Then the configure script could print that variable, and it could be re-used in src/Makefile.am. Seems like it would be less likely to get out of sync that wya.
243- fi
244-
245- if test "x$GXX" = xyes; then
246- CXXFLAGS="$CXXFLAGS -g3 -O0"
247- fi
248+ # Prefer -Og, fall back to -O0 if that is unavailable.
enable_debug section up two lines up, above the CXXFLAG_WERROR line. I realise you didn’t add this section here, but it is rewritten now, and seems out of place in the middle of the checks for compiler warning flags.
247- fi
248+ # Prefer -Og, fall back to -O0 if that is unavailable.
249+ AX_CHECK_COMPILE_FLAG(
250+ [-Og],
251+ [[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"]],
252+ [AX_CHECK_COMPILE_FLAG([-O0],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"]],,[[$CXXFLAG_WERROR]])],
$CXXFLAG_WERROR is intentional, or consider dropping it. It seem like it makes these checks unnecessarily verbose and also fragile.
@luke-jr Sorry, that was the explanation for a different change.
-D_FORTIFY has incompatibilities with -O0, and many toolchains forcibly set _FORTIFY. Newer glibc versions spew a warning in that case, which we want to catch here because it means that -O0 is unusable without unsetting _FORTIFY.
Of course, there’s the case to be made that –enable-debug and –enable-harden should be mutually exclusive, but as long as this is the only snag, I don’t see the need to go that far.
1379@@ -1370,9 +1380,9 @@ echo " build os = $BUILD_OS"
1380 echo
1381 echo " CC = $CC"
1382 echo " CFLAGS = $CFLAGS"
1383-echo " CPPFLAGS = $CPPFLAGS"
1384+echo " CPPFLAGS = $CPPFLAGS $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS"
Note the summary list gets formatted kind of weird with extra/leading spaces when options aren’t enabled, e.g. ./configure with no other options prints a summary like this for me:
0 CC = /usr/bin/ccache gcc
1 CFLAGS = -g -O2
2 CPPFLAGS = -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
3 CXX = /usr/bin/ccache g++ -std=c++11
4 CXXFLAGS = -Wstack-protector -fstack-protector-all -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter -Wno-implicit-fallthrough
5 LDFLAGS = -pthread -Wl,-z,relro -Wl,-z,now -pie
6 ARFLAGS = cr
The reason is that some flags we set will be missing or have whitespace themselves.
Here is a reasonable way to fix that problem in a way that is /bin/sh compatible and doesn’t use any non-Posix features: https://github.com/eklitzke/bitcoin/commit/fd35edc6e78eee2c2f9a6bca9409b3808affd726 . It technically doesn’t handle whitespace within parameters correctly, but I don’t think autoconf/automake does anyway. Unfortunately variable indirection in /bin/sh requires eval so you need to specify the flag name and its value separately. With that change the output looks like this:
0 CC = /usr/bin/ccache gcc
1 CFLAGS = -g -O2
2 CPPFLAGS = -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
3 CXX = /usr/bin/ccache g++ -std=c++11
4 CXXFLAGS = -Wstack-protector -fstack-protector-all -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter -Wno-implicit-fallthrough
5 LDFLAGS = -pthread -Wl,-z,relro -Wl,-z,now -pie
6 ARFLAGS = cr
Various changes:
* Don't check $GCC and $GXX
* Prefer -Og instead of -O0
* If -g3 isn't available, use -g
This also incidentally fixes compiler warnings with GCC and glibc when using
--enable-debug, as the old default values mixed poorly with the hardening flags.
That’s a good point, AX_APPEND_FLAG does in fact do the right thing wrt whitespace and feels a lot less hacky. We can let the output be a little untidy for now, I’ve got a 12 hour flight coming up in a few days and I might try to see if I can get AX_APPEND_FLAG and AX_APPEND_COMPILE_FLAGS working then.
Thanks for your patience reviewing this!
No problem, thanks for sticking with it!
The flags change sounds like the perfect thing to fill some idle flight time :)
Though, I think AX_APPEND_FLAG alone is probably enough. configure.ac is already so cryptic, it’s helpful for self-documentation to explicitly use CXXFLAGS rather than having it implied.
250+ [-Og],
251+ [[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"]],
252+ [AX_CHECK_COMPILE_FLAG([-O0],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"]],,[[$CXXFLAG_WERROR]])],
253+ [[$CXXFLAG_WERROR]])
254+
255+ # Prefer -g3, fall back to -g if that is unavailable.
-ggdb first…
Why not -glldb ? :)
Since we can’t anticipate, I think generic makes the most sense.
