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

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:trapv changing 2 files +2 −1
  1. practicalswift commented at 10:42 am on March 14, 2018: contributor
    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.
  2. fanquake added the label Tests on Mar 14, 2018
  3. fanquake commented at 2:36 pm on March 14, 2018: member

    HOST=x86_64-unknown-linux-gnu

     0The command "test -n "$USE_SHELL" && eval '"$USE_SHELL" -c "./autogen.sh"' || ./autogen.sh" exited with 0.
 1$ mkdir build && cd build
 2
 3The command "mkdir build && cd build" exited with 0.
 4$ ../configure --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( cat config.log && false)
 5configure: error: unrecognized option: `-ftrapv''
 6Try `../configure --help' for more information
 7cat: config.log: No such file or directory
 8
 9The command "../configure --cache-file=config.cache $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( cat config.log && false)" exited with 1.
10$ make distdir VERSION=$HOST
11make: *** No rule to make target `distdir'.  Stop.
  4. eklitzke commented at 10:36 pm on March 14, 2018: contributor

    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?

  5. eklitzke changes_requested
  6. eklitzke commented at 10:58 pm on March 14, 2018: contributor
    There’s a shell quoting issue with how travis is running this.
  7. eklitzke commented at 11:07 pm on March 14, 2018: contributor

    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?

  8. sipa commented at 11:47 pm on March 14, 2018: member
    @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.
  9. eklitzke commented at 6:36 am on March 15, 2018: contributor

    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
  10. practicalswift force-pushed on Mar 15, 2018
  11. practicalswift commented at 7:20 am on March 15, 2018: contributor

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

    Please re-review :-)

  12. practicalswift commented at 7:23 am on March 15, 2018: contributor
    @eklitzke Enabling -fsanitize=address and -fsanitize=thread in Travis (after cleaning up remaining violations) would be really really nice!
  13. eklitzke commented at 8:06 am on March 15, 2018: contributor

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

  14. practicalswift force-pushed on Mar 15, 2018
  15. practicalswift renamed this:
    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.
    on Mar 15, 2018
  16. practicalswift commented at 3:16 pm on March 15, 2018: contributor

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

  17. fanquake commented at 2:09 am on March 16, 2018: member
    @practicalswift I don’t think you’ve cherry-picked correctly, looks like you just swapped out the changes in your commit with @eklitzke’s ?
  18. eklitzke commented at 5:16 am on March 16, 2018: contributor
    This looks right to me, utACK e23dfbb01df044e6d0dc65f6b9333e6547202fa7.
  19. practicalswift commented at 5:46 pm on March 18, 2018: contributor
    @fanquake The cherry-pick was a previous version. e23dfbb01df044e6d0dc65f6b9333e6547202fa7 is correct. Please review :-)
  20. eklitzke commented at 3:26 am on March 22, 2018: contributor
    FYI this is going to conflict with #12695 . Should be an easy merge conflict though.
  21. laanwj assigned theuni on Apr 2, 2018
  22. laanwj commented at 1:12 pm on April 2, 2018: member
    @theuni Can you take a look?
  23. practicalswift force-pushed on Apr 14, 2018
  24. practicalswift commented at 1:09 pm on April 14, 2018: contributor
    Rebased! Please re-review :-)
  25. practicalswift force-pushed on Apr 14, 2018
  26. practicalswift force-pushed on Apr 16, 2018
  27. practicalswift commented at 9:10 am on April 23, 2018: contributor
    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!
  28. theuni commented at 5:19 pm on April 23, 2018: member

    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.

  29. in .travis.yml:35 in bb2d161746 outdated 
    29@@ -30,7 +30,7 @@ env:
30 # 32-bit + dash
31     - HOST=i686-pc-linux-gnu PACKAGES="g++-multilib python3-zmq" DEP_OPTS="NO_QT=1" RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-zmq --enable-glibc-back-compat --enable-reduce-exports LDFLAGS=-static-libstdc++" USE_SHELL="/bin/dash"
32 # x86_64 Linux (uses qt5 dev package instead of depends Qt to speed up build and avoid timeout)
33-    - HOST=x86_64-unknown-linux-gnu PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools protobuf-compiler libdbus-1-dev libharfbuzz-dev" DEP_OPTS="NO_QT=1 NO_UPNP=1 DEBUG=1 ALLOW_HOST_PACKAGES=1" RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports CPPFLAGS=-DDEBUG_LOCKORDER"
    theuni commented at 6:05 pm on April 23, 2018:

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

    +1 on using –enable-debug, though.

    practicalswift commented at 1:21 pm on May 5, 2018:
    Updated: Re-introduced DEBUG=1 :-)
  30. practicalswift force-pushed on May 5, 2018
  31. practicalswift force-pushed on May 14, 2018
  32. practicalswift force-pushed on May 14, 2018
  33. practicalswift commented at 2:02 pm on May 14, 2018: contributor

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

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

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

  34. theuni commented at 10:51 pm on May 15, 2018: member
    Please add the CXXFLAG_WERROR argument so that we’ll see false-positives. utACK otherwise.
  35. practicalswift force-pushed on May 16, 2018
  36. practicalswift force-pushed on May 16, 2018
  37. practicalswift commented at 7:28 am on May 16, 2018: contributor
    @TheBlueMatt Added the CXXFLAG_WERROR argument. Please re-review :-)
  38. practicalswift force-pushed on May 29, 2018
  39. practicalswift commented at 10:06 pm on May 29, 2018: contributor
    Rebased!
  40. theuni commented at 10:33 pm on June 12, 2018: member
    utACK 8bf481fe2e99e6a0e16a6127d7df826dd4bfa2e9
  41. DrahtBot added the label Needs rebase on Jun 24, 2018
  42. DrahtBot commented at 2:21 pm on June 24, 2018: member
  43. Add -ftrapv to DEBUG_CXXFLAGS when --enable-debug is used 94e52d13db
  44. travis: Build with --enable-debug (x86_64-unknown-linux-gnu) 98d842cb52
  45. practicalswift force-pushed on Jun 24, 2018
  46. practicalswift commented at 6:36 pm on June 24, 2018: contributor
    Rebased!
  47. MarcoFalke commented at 6:41 pm on June 24, 2018: member
    ACK 98d842cb52 (that the rebase was done correctly, didn’t look at anything else)
  48. DrahtBot removed the label Needs rebase on Jun 24, 2018
  49. sipa commented at 0:54 am on June 27, 2018: member
    utACK 98d842cb52e69ea1f9961d908385c896e37fb877
  50. sipa merged this on Jun 27, 2018
  51. sipa closed this on Jun 27, 2018
  52. sipa referenced this in commit 2643fa5086 on Jun 27, 2018
  53. MarcoFalke referenced this in commit 7a9bca61fa on Jul 24, 2018
  54. zkbot referenced this in commit 8e8a9350c3 on Jan 30, 2020
  55. MarcoFalke referenced this in commit 7027c67cac on Jul 2, 2020
  56. practicalswift deleted the branch on Apr 10, 2021
  57. UdjinM6 referenced this in commit 5836a28644 on Jun 29, 2021
  58. UdjinM6 referenced this in commit a0426da513 on Jun 29, 2021
  59. UdjinM6 referenced this in commit ad039fd4f0 on Jul 1, 2021
  60. vijaydasmp referenced this in commit dfa262c93f on Oct 4, 2021
  61. Munkybooty referenced this in commit 3285e21cfd on Apr 26, 2022
  62. DrahtBot locked this on Aug 16, 2022


practicalswift fanquake eklitzke sipa laanwj theuni DrahtBot MarcoFalke

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me