Skip to content

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jul 25, 2017

Build with --enable-werror under OS X.

As suggested by @TheBlueMatt in #10866. This will allow catching violations using Travis CI which does a clang build for OS X.

@practicalswift practicalswift changed the title Use -Wthread-safety-analysis if available (+ -Werror=thread-safety-analysis if --enable-werror) Use -Wthread-safety-analysis if available (+ -Werror=[…] if --enable-werror) Jul 25, 2017
@TheBlueMatt
Copy link
Contributor

Looks good to me, though I'm confused, in #10866 you indicated that these checks should fail, but travis passed here?

@practicalswift
Copy link
Contributor Author

practicalswift commented Jul 25, 2017

@TheBlueMatt Yes, that is a bit weird. I verified this change locally using these four tests before submitting the PR:

$ CC=clang CXX=clang++ ./configure --enable-werror && make clean && make check; echo $?# fails as expected:
Makefile:5576: recipe for target 'libbitcoin_server_a-net_processing.o' failed
2
$ CC=clang CXX=clang++ ./configure && make clean && make check; echo $?# passes as expected:
0
$ CC=gcc CXX=g++ ./configure --enable-werror && make clean && make check; echo $?# passes as expected:
0
$ CC=gcc CXX=g++ ./configure && make clean && make check; echo $?# passes as expected:
0
$ clang++ --version | head -1
clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)

Locally everything works as expected.

Does the Travis CI build run with --enable-werror? I cannot find that in .travis.yml.

@maflcko
Copy link
Member

maflcko commented Jul 30, 2017

No, --enable-werror is currently not set on travis.

@TheBlueMatt
Copy link
Contributor

Great! Lets enable it by default at least on the osx build. Its deliberately heavily limited so no reason not to (also maybe do it by default, @theuni?).

@theuni
Copy link
Member

theuni commented Jul 31, 2017

Yea, I like this!

My only concern is that the annotations are infectious, so this may cause us to have to add them while doing large refactors where one annotation already exists. That's a good problem to have though, IMO.

utACK after #10866 goes in.

@TheBlueMatt
Copy link
Contributor

Looks like the travis failure was from building fontconfig, not the expected annotation-missing failure. Is it unrelated?

@practicalswift
Copy link
Contributor Author

@TheBlueMatt Yes it appears to fail before hitting expected annotation-missing failure. I'm not sure about the root cause here. Can we as a first step re-trigger the Travis CI build?

@TheBlueMatt
Copy link
Contributor

@practicalswift OK, it has been kicked, I dont know how building fontconfig would have failed because of this, but hopefully its deterministic if it is an issue with this PR.

@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 15, 2017

@TheBlueMatt Travis CI now complains about a locking issue but it is not the locking issue I'm encountering locally (locally I'm getting the error described in #10866). Is the locking issue reported by Travis a false positive?

@TheBlueMatt
Copy link
Contributor

@practicalswift hmm, well I got it to work by changing the sync.h primitives to std from boost (see https://travis-ci.org/TheBlueMatt/bitcoin/jobs/265262602 and https://github.com/TheBlueMatt/bitcoin/commits/2017-08-test-10923)

@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 16, 2017

@TheBlueMatt Oh, nice! What is the best way to proceed from here?

Should the changing of sync.h primitives from boost to std be put in a separate PR of yours, or should I simply incorporate your changes into this PR?

These are the changes: practicalswift/bitcoin@thread-safety-analysis...TheBlueMatt:2017-08-test-10923

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Aug 18, 2017 via email

@practicalswift
Copy link
Contributor Author

@TheBlueMatt What meeting? What am I missing? :-)

@practicalswift practicalswift force-pushed the thread-safety-analysis branch from 9df8538 to e1abd17 Compare August 18, 2017 13:53
@practicalswift practicalswift changed the title Use -Wthread-safety-analysis if available (+ -Werror=[…] if --enable-werror) Use -Wthread-safety-analysis if available Aug 18, 2017
@practicalswift
Copy link
Contributor Author

@TheBlueMatt PR updated. Looks good? :-)

@TheBlueMatt
Copy link
Contributor

@ptracticalswift the IRC meeting, Werror was discussed. Change the PR title?

@TheBlueMatt
Copy link
Contributor

@practicalswift wait, huh, I meant just do Werror here, and the thread-safety-analysis thing can go in #10866.

@practicalswift practicalswift force-pushed the thread-safety-analysis branch from e1abd17 to a7433f0 Compare August 19, 2017 14:20
@practicalswift practicalswift force-pushed the thread-safety-analysis branch from a7433f0 to a65e028 Compare August 19, 2017 14:23
@practicalswift practicalswift changed the title Use -Wthread-safety-analysis if available Build with --enable-werror under OS X Aug 19, 2017
@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 19, 2017

@TheBlueMatt Got it! I haven't attended any Bitcoin IRC channels/meetings - are there logs available somewhere? :-)

@fanquake
Copy link
Member

@TheBlueMatt
Copy link
Contributor

ACK

@jonasschnelli
Copy link
Contributor

ACK a65e028

@laanwj laanwj changed the title Build with --enable-werror under OS X travis: Build with --enable-werror under OS X Aug 23, 2017
@laanwj
Copy link
Member

laanwj commented Aug 23, 2017

Prefixed the issue title with "travis:" otherwise it looks like all OSX builds will enable -werror, which is not the intent.

@laanwj laanwj merged commit a65e028 into bitcoin:master Aug 23, 2017
laanwj added a commit that referenced this pull request Aug 23, 2017
a65e028 Build with --enable-werror under OS X (practicalswift)

Pull request description:

  Build with `--enable-werror` under OS X.

  As suggested by @TheBlueMatt in #10866. This will allow catching violations using Travis CI which does a `clang` build for OS X.

Tree-SHA512: 326248897e0776106983e0824e7e80eee3c6e584a1d360f429c30f3375dad83ab4c360c86ab0729bd9ede747ea0caa13cd6a7c35072ed9b845362423c9c37a64
maflcko pushed a commit that referenced this pull request Jul 2, 2020
…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
@practicalswift practicalswift deleted the thread-safety-analysis branch April 10, 2021 19:32
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Oct 4, 2021
…etect use 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 (bitcoin#10866, bitcoin#10923)
  * 2018: Continuous testing with trapping on signed integer overflows (`-ftrapv`) (bitcoin#12686)
  * 2018: Continuous testing of use of locale dependent functions (bitcoin#13041)
  * 2018: Continuous testing of format strings (bitcoin#13705)
  * 2018: Continuous compilation with MSVC `TreatWarningAsError` (bitcoin#14151)
  * 2018: Continuous testing under UndefinedBehaviorSanitizer – UBSan (bitcoin#14252, bitcoin#14673, bitcoin#17006)
  * 2018: Continuous testing under AddressSanitizer – ASan (bitcoin#14794, bitcoin#17205, bitcoin#17674)
  * 2018: Continuous testing under ThreadSanitizer – TSan (bitcoin#14829)
  * 2019: Continuous testing in an unsigned char environment (`-funsigned-char`) (bitcoin#15134)
  * 2019: Continuous compile-time testing of assumptions we're making (bitcoin#15391)
  * 2019: Continuous testing of fuzz test cases under Valgrind (bitcoin#17633, bitcoin#18159, bitcoin#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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants