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

By generating a trap for signed overflow on addition, subtraction, multiplication operations in the Travis testing we are more likely to identify problematic code prior to merging it.

HOST=x86_64-unknown-linux-gnu

The command "test -n "$USE_SHELL" && eval '"$USE_SHELL" -c "./autogen.sh"' || ./autogen.sh" exited with 0.
$ mkdir build && cd build

The command "mkdir build && cd build" exited with 0.
$ ../configure --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( cat config.log && false)
configure: error: unrecognized option: `-ftrapv''
Try `../configure --help' for more information
cat: config.log: No such file or directory

The command "../configure --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( cat config.log && false)" exited with 1.
$ make distdir VERSION=$HOST
make: *** No rule to make target `distdir'.  Stop.

This is cool, concept ack.

Looks like there are some problems:

• -fwrapv belongs in CXXFLAGS not in CPPFLAGS
• Travis is being weird. The builders that passed seem to have used some kind of cached config and didn't actually pick up -ftrapv. The one that's failing is failing because there's a shell quoting issue with how configure is invoked.

GCC 4.8 also supports -fsanitize=address and -fsanitize=thread (and newer GCC versions have a whole plethora or really interesting options). What do you think about using those options in Travis as well?

There's a shell quoting issue with how travis is running this.

Here's an idea for another way to do this: https://github.com/eklitzke/bitcoin/commit/635e378383c41c8ef3ac03fca1755001c947b7f7 . The idea is to add -ftrapv when --enable-debug is used, and then use that option on all of the Travis jobs. --enable-debug also sets -DDEBUG_LOCKORDER.

What do you think of this approach?

@eklitzke There was some work earlier to get the code to run cleanly under sanitizers (see #9743 and #9512), though I don't think we ever made it perfectly runnable without warnings.

BTW the right way to fix the quoting issue in Travis is to use a bash array. It looks like Travis doesn't support these properly. If you still wanted to use this approach instead of modifying configure.ac as in my example two options:

• IFS hacks
• Create yet-another-Travis-variable and pass it down to the configure call

@eklitzke Thanks for reviewing. I've now cherry-picked your commit (which is the better solution) into this PR.

Please re-review :-)

@eklitzke Enabling -fsanitize=address and -fsanitize=thread in Travis (after cleaning up remaining violations) would be really really nice!

It looks like this doesn't work for the MingW builds, I got timeouts in my travis runs that match the ones you just cherry-picked: https://travis-ci.org/eklitzke/bitcoin/builds/353587968 . Let's disable the option for those two builders.

I added some -fsanitize support to the configure script in #12692, which is a good start since it will make using those flags easier (even if the code isn't currently clean under asan/tsan).

@eklitzke To keep the changes as small as possible I've now enabled -ftrapv (via --enable-debug) only for one of the Travis jobs (job: "Qt4 & system libs"). Makes sense?

Please re-review :-)

@practicalswift I don't think you've cherry-picked correctly, looks like you just swapped out the changes in your commit with @eklitzke's ?

This looks right to me, utACK e23dfbb01df044e6d0dc65f6b9333e6547202fa7.

@fanquake The cherry-pick was a previous version. e23dfbb01df044e6d0dc65f6b9333e6547202fa7 is correct. Please review :-)

FYI this is going to conflict with #12695 . Should be an easy merge conflict though.

@theuni Can you take a look?

Rebased! Please re-review :-)

Interesting – judging from the Travis testing it seems that the trap is trigged indicating that we have a signed overflow taking place when running the tests. Let's find out where!

Concept ACK. Please put this on top of #13005 though, as it needs the same treatment.

This needs to be checked with AX_CHECK_COMPILE_FLAG, the same way that -DDEBUG/-DDEBUG_LOCKORDER are checked there.

Now using AX_CHECK_COMPILE_FLAG for checking -trapv availability when compiled under --enable-debug:

$ ./configure --enable-debug
…
checking whether C++ compiler accepts -ftrapv... yes
…
  CXXFLAGS      =  -Og -g3 -ftrapv  -Wstack-protector -fstack-protector-all   -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter
…

@theuni @eklitzke, please re-review :-)

Please add the CXXFLAG_WERROR argument so that we'll see false-positives. utACK otherwise.

@TheBlueMatt Added the CXXFLAG_WERROR argument. Please re-review :-)

Rebased!

utACK 8bf481fe2e99e6a0e16a6127d7df826dd4bfa2e9

<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

Rebased!

ACK 98d842cb52 (that the rebase was done correctly, didn't look at anything else)

utACK 98d842cb52e69ea1f9961d908385c896e37fb877