Add -ftrapv to CFLAGS and CXXFLAGS when --enable-debug is used. Enable -ftrapv in Travis. by practicalswift · Pull Request #12686 · bitcoin/bitcoin

practicalswift · 2018-03-14T10:42:39Z

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.

fanquake · 2018-03-14T14:36:04Z

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.

eklitzke · 2018-03-14T22:36:09Z

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?

eklitzke

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

eklitzke · 2018-03-14T23:07:41Z

Here's an idea for another way to do this: eklitzke@635e378 . 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?

sipa · 2018-03-14T23:47:54Z

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

eklitzke · 2018-03-15T06:36:11Z

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

practicalswift · 2018-03-15T07:20:50Z

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

Please re-review :-)

practicalswift · 2018-03-15T07:23:16Z

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

eklitzke · 2018-03-15T08:06:22Z

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

practicalswift · 2018-03-15T15:16:08Z

@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 :-)

fanquake · 2018-03-16T02:09:48Z

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

eklitzke · 2018-03-16T05:16:03Z

This looks right to me, utACK e23dfbb01df044e6d0dc65f6b9333e6547202fa7.

practicalswift · 2018-03-18T17:46:21Z

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

eklitzke · 2018-03-22T03:26:45Z

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

laanwj · 2018-04-02T13:12:54Z

@theuni Can you take a look?

practicalswift · 2018-04-14T13:09:19Z

Rebased! Please re-review :-)

practicalswift · 2018-04-23T09:10:28Z

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!

theuni · 2018-04-23T17:19:11Z

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.

theuni · 2018-04-23T18:05:41Z

.travis.yml

Removing DEBUG=1 here should cause depends to no longer build as debug.

+1 on using --enable-debug, though.

Updated: Re-introduced DEBUG=1 :-)

DrahtBot · 2018-06-24T14:21:33Z

Needs rebase

practicalswift · 2018-06-24T18:36:56Z

Rebased!

maflcko · 2018-06-24T18:41:07Z

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

sipa · 2018-06-27T00:54:01Z

utACK 98d842c

…is used. Enable -ftrapv in Travis. 98d842c travis: Build with --enable-debug (x86_64-unknown-linux-gnu) (practicalswift) 94e52d1 Add -ftrapv to DEBUG_CXXFLAGS when --enable-debug is used (practicalswift) Pull request description: 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. Tree-SHA512: 47712da53b4ff451b8f22f16ddc3b53100a09060a3b04cda4b8fbbb74e6f666fc07a9cc7abc64cacb87a0aa3f62dc8e3c91a1a0ed12bf82bb2a5624a5d104389

14788fb [travis] Don't store debug info if --enable-debug is set (Chun Kuan Lee) Pull request description: After #12686 merged, ccache store huge size of .o files, simply get rid of those useless debug info. Fixes #13748 Tree-SHA512: fb404e2c7d52cd8266548433955c41683ede062f97c8fb7098a887f164bcde48b60e5e533a0a27c7e095fdd9ef88db018b8689adebb2c0e32c8957828629e346

./configure updates Includes code cherry-picked from the following upstream Bitcoin Core PRs: - bitcoin/bitcoin#6748 - bitcoin/bitcoin#12373 - bitcoin/bitcoin#12692 - bitcoin/bitcoin#12901 - bitcoin/bitcoin#13005 - bitcoin/bitcoin#13445 - bitcoin/bitcoin#12686 - bitcoin/bitcoin#16435 Part of #2074.

…se of uninitialized memory 870f0cd build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory (practicalswift) Pull request description: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory. First UBSan, then ASan followed by TSan... and now: yes, the wait is over -- **MSan is finally here!** :) Some historical context: * 2017: Continuous compilation with Clang Thread Safety analysis enabled (#10866, #10923) * 2018: Continuous testing with trapping on signed integer overflows (`-ftrapv`) (#12686) * 2018: Continuous testing of use of locale dependent functions (#13041) * 2018: Continuous testing of format strings (#13705) * 2018: Continuous compilation with MSVC `TreatWarningAsError` (#14151) * 2018: Continuous testing under UndefinedBehaviorSanitizer – UBSan (#14252, #14673, #17006) * 2018: Continuous testing under AddressSanitizer – ASan (#14794, #17205, #17674) * 2018: Continuous testing under ThreadSanitizer – TSan (#14829) * 2019: Continuous testing in an unsigned char environment (`-funsigned-char`) (#15134) * 2019: Continuous compile-time testing of assumptions we're making (#15391) * 2019: Continuous testing of fuzz test cases under Valgrind (#17633, #18159, #18166) * 2020: Finally... MemorySanitizer – MSAN! :) What is the next step? What tools should we add to CI to keep bugs from entering `master`? :) ACKs for top commit: MarcoFalke: ACK 870f0cd Tree-SHA512: 38327c8b75679d97d469fe42e704cacd1217447a5a603701dd8a58ee50b3be2c10248f8d68a479ed081c0c4b254589d3081c9183f991640b06ef689061f75578

…-debug is used. Enable -ftrapv in Travis. 98d842c travis: Build with --enable-debug (x86_64-unknown-linux-gnu) (practicalswift) 94e52d1 Add -ftrapv to DEBUG_CXXFLAGS when --enable-debug is used (practicalswift) Pull request description: 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. Tree-SHA512: 47712da53b4ff451b8f22f16ddc3b53100a09060a3b04cda4b8fbbb74e6f666fc07a9cc7abc64cacb87a0aa3f62dc8e3c91a1a0ed12bf82bb2a5624a5d104389

fanquake added the Tests label Mar 14, 2018

practicalswift mentioned this pull request Mar 14, 2018

interpreter: Use safer casting in IsValidSignatureEncoding(...) #12611

Closed

eklitzke suggested changes Mar 14, 2018

View reviewed changes

eklitzke mentioned this pull request Mar 15, 2018

Enable -fsanitize flags in Travis #12691

Closed

practicalswift force-pushed the trapv branch from fa7d989 to a0cf01b Compare March 15, 2018 07:19

practicalswift force-pushed the trapv branch from a0cf01b to e23dfbb Compare March 15, 2018 15:14

practicalswift changed the title ~~travis: Generate a trap for signed arithmetic overflows (enable -ftrapv)~~ Add -ftrapv to CFLAGS and CXXFLAGS when --enable-debug is used. Enable -ftrapv in Travis. Mar 15, 2018

laanwj assigned theuni Apr 2, 2018

practicalswift force-pushed the trapv branch from e23dfbb to c37821b Compare April 14, 2018 13:08

practicalswift force-pushed the trapv branch 2 times, most recently from 3fc534a to bb2d161 Compare April 16, 2018 19:55

theuni reviewed Apr 23, 2018

View reviewed changes

practicalswift force-pushed the trapv branch from bb2d161 to 4771482 Compare May 5, 2018 13:20

DrahtBot added the Needs rebase label Jun 24, 2018

practicalswift added 2 commits June 24, 2018 20:35

Add -ftrapv to DEBUG_CXXFLAGS when --enable-debug is used

94e52d1

travis: Build with --enable-debug (x86_64-unknown-linux-gnu)

98d842c

practicalswift force-pushed the trapv branch from 8bf481f to 98d842c Compare June 24, 2018 18:36

DrahtBot removed the Needs rebase label Jun 24, 2018

sipa merged commit 98d842c into bitcoin:master Jun 27, 2018

Bushstar mentioned this pull request Jun 28, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#350

Merged

This was referenced Jul 12, 2018

[travis] Increase ccache size for the one which enable debug #13648

Closed

[travis] Don't store debug info if --enable-debug is set #13650

Merged

str4d mentioned this pull request Nov 12, 2019

./configure updates zcash/zcash#4222

Merged

practicalswift mentioned this pull request Mar 7, 2020

build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

Merged

practicalswift deleted the trapv branch April 10, 2021 19:35

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

practicalswift commented Mar 14, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

eklitzke left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

sipa commented Mar 14, 2018

Uh oh!

eklitzke commented Mar 15, 2018

Uh oh!

practicalswift commented Mar 15, 2018

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!

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!

fanquake commented Mar 16, 2018

Uh oh!

eklitzke commented Mar 16, 2018

Uh oh!

practicalswift commented Mar 18, 2018

Uh oh!

eklitzke commented Mar 22, 2018

Uh oh!

laanwj commented Apr 2, 2018

Uh oh!

practicalswift commented Apr 14, 2018

Uh oh!

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

Uh oh!

Uh oh!

theuni commented Apr 23, 2018

Uh oh!

theuni Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 24, 2018

Uh oh!

practicalswift commented Jun 24, 2018

Uh oh!

maflcko commented Jun 24, 2018

Uh oh!

sipa commented Jun 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fanquake commented Mar 14, 2018 •

edited

Loading

eklitzke commented Mar 14, 2018 •

edited

Loading

eklitzke commented Mar 14, 2018 •

edited

Loading

practicalswift commented Mar 15, 2018 •

edited

Loading

eklitzke commented Mar 15, 2018 •

edited

Loading

practicalswift commented Mar 15, 2018 •

edited

Loading

practicalswift commented Apr 23, 2018 •

edited

Loading

practicalswift May 5, 2018 •

edited

Loading