[build] Make --enable-debug pick better options #12695

eklitzke · 2018-03-15T12:19:53Z

This changes --enable-debug to choose better options:

We use -Og rather than -O0 if it is available
Prefer -g3 over -g if it is available
Add debug preprocessor flags to $DEBUG_CPPFLAGS rather than updating $CPPFLAGS directly

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

practicalswift · 2018-03-15T14:12:56Z

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!

theuni

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.

AX_CHECK_COMPILE_FLAG([-Og],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"]],,[[$CXXFLAG_WERROR]])
AX_CHECK_COMPILE_FLAG([-g3],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -g3"]],,[[$CXXFLAG_WERROR]])

AX_CHECK_PREPROC_FLAG([-DDEBUG],[[DEBUG_CPPFLAGS="$DEBUG_CPPFLAGS -DDEBUG"]],,[[$CXXFLAG_WERROR]])
AX_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.

eklitzke · 2018-03-16T18:50:03Z

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:

Recommends you use -Og
Says that -g3 includes macro definitions in the debug symbols (the only difference from -g2, which is the default when using -g)

Clang:

"-O0 Means "no optimization": this level compiles the fastest and generates the most debuggable code"
"Note that Clang debug information works best at -O0."
"-Og Like -O1. In future versions, this option might disable different optimizations in order to improve debuggability."
Option -g3 is not documented in the man page.

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.

eklitzke · 2018-03-16T18:58:57Z

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.

[1] https://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_append_compile_flags.m4

eklitzke · 2018-03-16T19:30:54Z

Summary of recent commits:

0014486f25cfc628d79939f55adb6e020fbf0173 changes you suggested
b59e74db3e013a3551ef8d2df889fe26ea2d0a3c make the summary at the end of configure show all the flags we're actually using
ed6066b055ca5d024738214a16c91be9cad5c54d proposed way for adding different flags in clang (not sure if you think adding this kind of logic is appropriate or not, just an idea)

theuni · 2018-03-20T19:55:35Z

configure.ac

-  AX_CHECK_PREPROC_FLAG([-D_FORTIFY_SOURCE=2],[
-    AX_CHECK_PREPROC_FLAG([-U_FORTIFY_SOURCE],[
-      HARDENED_CPPFLAGS="$HARDENED_CPPFLAGS -U_FORTIFY_SOURCE"
+  dnl -D_FORTIFY_SOURCE=1 and 2 incompatible with -O0


This should be automatically detected where necessary (where it would otherwise be warning) if we turn warnings into errors by passing $CXXFLAG_WERROR as the last argument to AX_CHECK_PREPROC_FLAG.

Clever, good idea.

theuni · 2018-03-20T19:58:56Z

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:

if test $CLANG -eq 1; then
  AX_CHECK_COMPILE_FLAG([-O0],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"]],,[[$CXXFLAG_WERROR]])
...

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:

gcc's -Og is also basically -O1, with a few things explicitly removed:

    { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fbranch_count_reg, NULL, 1 },
    { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fmove_loop_invariants, NULL, 1 },
    { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_ftree_pta, NULL, 1 },
    { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fssa_phiopt, NULL, 1 },

clang's -gX (other than -g1) are aliased to '-g', so, same behavior as gcc minus the macro definitions.

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:

-Og with a fallback to -O1 if it doesn't work
-g3 with a fallback to -g if it doesn't work.

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.

eklitzke · 2018-03-20T21:35:08Z

Changes here:

Remove the GCC/Clang check
Try -Og, fall back to -O0
Try -g3, fall back to -g
Pass in debug/error flags when doing the _FORTIFY_SOURCE check

There's one other change here that I would like @luke-jr to review. In 9b4e03b 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.

theuni · 2018-03-20T22:38:57Z

configure.ac

@@ -592,12 +601,16 @@ if test x$use_hardening != xno; then
  AX_CHECK_COMPILE_FLAG([-Wstack-protector],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -Wstack-protector"])
  AX_CHECK_COMPILE_FLAG([-fstack-protector-all],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fstack-protector-all"])

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

OK, will undo this.

theuni · 2018-03-20T22:42:38Z

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.

eklitzke · 2018-03-20T23:06:12Z

Reverted the change to _FORTIFY_SOURCE, now the only difference in that part of the file compared to master is that we pass in debug and error flags when making the check. If this looks OK I can squash the branch down.

eklitzke · 2018-03-20T23:08:59Z

Hmm it appears the extra flags are supposed to be CPPFLAGS not CFLAGS/CXXFLAGS according to the m4 macro. So maybe there's no point in passing in the flags at all there. Kind of a bummer since that means it doesn't catch the interaction with -O0.

eklitzke · 2018-03-20T23:15:44Z

Removed the _FORTIFY_SOURCE changes altogether since the new check isn't effective. The only reason I wanted to change it in the first place was because it interacted poorly with -O0, and on the platforms that generates warnings on we no longer expect to see warnings.

eklitzke · 2018-03-20T23:31:20Z

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:

AM_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).

theuni · 2018-03-20T23:38:55Z

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.

theuni · 2018-03-20T23:48:26Z

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 e0077de 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).

eklitzke · 2018-03-21T00:46:53Z

I think that last test failure was Travis being flaky, b0860d8e82041786b3424851b42a8b8c5bc143e8 squashes all of these changes down.

ryanofsky

utACK b0860d8e82041786b3424851b42a8b8c5bc143e8

ryanofsky · 2018-03-22T13:37:02Z

configure.ac

 echo "  CXX           = $CXX"
-echo "  CXXFLAGS      = $CXXFLAGS"
-echo "  LDFLAGS       = $LDFLAGS"
+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.

Agreed. I haven't complained about this becoming stale because it seemed inevitable that it would be non-exact due to the per-object and per-library flags. But keeping it in sync with AM_*FLAGS makes sense.

Updated. What about something like:

DEFAULT_AM_CXXFLAGS="$CXXFLAGS $DEBUG_CXXFLAGS etc..." AC_SUBST(DEFAULT_AM_CXXFLAGS) # 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.

ryanofsky · 2018-03-22T13:55:33Z

configure.ac

-    if test "x$GXX" = xyes; then
-        CXXFLAGS="$CXXFLAGS -g3 -O0"
-    fi
+  # Prefer -Og, fall back to -O0 if that is unavailable.


Maybe move this 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.

enable_debug uses CXXFLAG_WERROR, so it needs to be below the definition.

ryanofsky · 2018-03-22T13:57:46Z

configure.ac

+  AX_CHECK_COMPILE_FLAG(
+    [-Og],
+    [[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"]],
+    [AX_CHECK_COMPILE_FLAG([-O0],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"]],,[[$CXXFLAG_WERROR]])],


Maybe add a comment here if passing if $CXXFLAG_WERROR is intentional, or consider dropping it. It seem like it makes these checks unnecessarily verbose and also fragile.

It's passed on purpose to turn -D_FORTIFY re-definition warnings into errors.

Why would we want an error in that case?

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

ryanofsky · 2018-03-22T14:03:14Z

configure.ac

@@ -1370,9 +1380,9 @@ echo "  build os      = $BUILD_OS"
 echo
 echo "  CC            = $CC"
 echo "  CFLAGS        = $CFLAGS"
-echo "  CPPFLAGS      = $CPPFLAGS"
+echo "  CPPFLAGS      = $CPPFLAGS $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS"


CPPFLAGS and CXXFLAGS should probably be printed after the other flags instead of before, since they will override them.

ryanofsky · 2018-03-22T14:08:25Z

PR description #12695 (comment) should be updated to match commit description, since it seems out of date now.

eklitzke · 2018-03-27T03:27:58Z

Updated the configure summary at the end, and changed PR description.

eklitzke · 2018-03-27T03:52:44Z

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:

  CC            = /usr/bin/ccache gcc
  CFLAGS        = -g -O2
  CPPFLAGS      =   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
  CXX           = /usr/bin/ccache g++ -std=c++11
  CXXFLAGS      =   -Wstack-protector -fstack-protector-all   -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter -Wno-implicit-fallthrough
  LDFLAGS       = -pthread  -Wl,-z,relro -Wl,-z,now -pie  
  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: eklitzke@fd35edc . 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:

  CC            = /usr/bin/ccache gcc
  CFLAGS        = -g -O2
  CPPFLAGS      = -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
  CXX           = /usr/bin/ccache g++ -std=c++11
  CXXFLAGS      = -Wstack-protector -fstack-protector-all -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter -Wno-implicit-fallthrough
  LDFLAGS       = -pthread -Wl,-z,relro -Wl,-z,now -pie
  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.

theuni · 2018-03-28T20:51:19Z

I think switching to AX_APPEND_FLAG / AX_APPEND_COMPILE_FLAGS takes care of the whitespace problem more elegantly?

theuni

utACK

eklitzke · 2018-03-29T00:00:04Z

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!

theuni · 2018-03-29T17:49:47Z

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.

luke-jr · 2018-03-29T17:52:15Z

configure.ac

+    [AX_CHECK_COMPILE_FLAG([-O0],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"]],,[[$CXXFLAG_WERROR]])],
+    [[$CXXFLAG_WERROR]])
+
+  # Prefer -g3, fall back to -g if that is unavailable.


I wonder if it should prefer -ggdb first...

Why not -glldb ? :)

Since we can't anticipate, I think generic makes the most sense.

Right, that kind of option makes more sense to pass in manually if you need it.

laanwj · 2018-03-29T21:52:28Z

utACK - needs rebase though (probably due to your own commit, 6feb46c :).

ryanofsky

utACK 9418964. Only change since last review is correcting CXXFLAGS/LDFLAG echo lines

fanquake · 2018-04-17T08:52:50Z

Rebased in #13005.

9e49db2 Make --enable-debug to pick better options (Evan Klitzke) Pull request description: Cherry-picked (and rebased) 9418964 from the "up for grabs" PR: "[build] Make --enable-debug pick better options" (#12695). See previous review in #12695. Tree-SHA512: a93cdadcf13e2ef8519acb1ce4f41ce95057a388347bb0a86a5c164dc7d0b0d14d4bb2a466082d5a100b8d50de65c605c40abaed555e8ea77c99e28800a34439

9e49db2 Make --enable-debug to pick better options (Evan Klitzke) Pull request description: Cherry-picked (and rebased) 9418964 from the "up for grabs" PR: "[build] Make --enable-debug pick better options" (bitcoin#12695). See previous review in bitcoin#12695. Tree-SHA512: a93cdadcf13e2ef8519acb1ce4f41ce95057a388347bb0a86a5c164dc7d0b0d14d4bb2a466082d5a100b8d50de65c605c40abaed555e8ea77c99e28800a34439

fanquake added the Build system label Mar 15, 2018

maflcko requested a review from theuni March 15, 2018 12:45

theuni suggested changes Mar 15, 2018

View reviewed changes

eklitzke mentioned this pull request Mar 16, 2018

Add configure options for various -fsanitize flags #12692

Merged

theuni reviewed Mar 20, 2018

View reviewed changes

eklitzke force-pushed the enabledebug branch from 2f043fd to b0860d8 Compare March 21, 2018 00:45

eklitzke mentioned this pull request Mar 22, 2018

Add -ftrapv to CFLAGS and CXXFLAGS when --enable-debug is used. Enable -ftrapv in Travis. #12686

Merged

ryanofsky reviewed Mar 22, 2018

View reviewed changes

eklitzke force-pushed the enabledebug branch from b0860d8 to c5483a0 Compare March 27, 2018 03:22

eklitzke changed the title ~~[build] Fix some strange behavior with --enable-debug~~ [build] Make --enable-debug pick better options Mar 27, 2018

eklitzke force-pushed the enabledebug branch from c5483a0 to 9418964 Compare March 28, 2018 00:55

theuni approved these changes Mar 28, 2018

View reviewed changes

luke-jr reviewed Mar 29, 2018

View reviewed changes

maflcko added the Up for grabs label Apr 8, 2018

ryanofsky reviewed Apr 16, 2018

View reviewed changes

practicalswift mentioned this pull request Apr 17, 2018

Make --enable-debug to pick better options #13005

Merged

fanquake closed this Apr 17, 2018

maflcko removed the Up for grabs label Apr 17, 2018

practicalswift mentioned this pull request Nov 11, 2018

Fix icc (Intel C++ compiler) warnings #14706

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[build] Make --enable-debug pick better options #12695

[build] Make --enable-debug pick better options #12695

Uh oh!

Conversation

eklitzke commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

eklitzke commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eklitzke commented Mar 16, 2018

Uh oh!

eklitzke commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Mar 20, 2018

Uh oh!

eklitzke commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Mar 20, 2018

Uh oh!

eklitzke commented Mar 20, 2018

Uh oh!

eklitzke commented Mar 20, 2018

Uh oh!

eklitzke commented Mar 20, 2018

Uh oh!

eklitzke commented Mar 20, 2018

Uh oh!

theuni commented Mar 20, 2018

Uh oh!

theuni commented Mar 20, 2018

Uh oh!

eklitzke commented Mar 21, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Mar 22, 2018

Uh oh!

eklitzke commented Mar 27, 2018

Uh oh!

eklitzke commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eklitzke commented Mar 15, 2018 •

edited

Loading

practicalswift commented Mar 15, 2018 •

edited

Loading

eklitzke commented Mar 16, 2018 •

edited

Loading

eklitzke commented Mar 27, 2018 •

edited

Loading