-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmake: Amend docs per feedback #316
Conversation
A few amendment to fuzzing.md 1. Removed non-portable $(nproc), mirrors the master branch. 2. Removed stray back slash. 3. Added a linker workaround for macOS.
cc @dergoegge @m3dwards @paplorinc |
@@ -150,6 +150,7 @@ $ cmake -B build_fuzz \ | |||
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \ | |||
-DBUILD_FOR_FUZZING=ON \ | |||
-DSANITIZERS=undefined,address,fuzzer \ | |||
-DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the thread from the other PR, it's not clear if there is an upstream issue open, or someone is going to open one. I see links to other discussions that look similar, but from what I can tell, aren't our issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, left a few comments that I'd like us to consider
@@ -171,9 +172,9 @@ $ cmake -B build_fuzz \ | |||
-DCMAKE_C_COMPILER="$(pwd)/AFLplusplus/afl-clang-lto" \ | |||
-DCMAKE_CXX_COMPILER="$(pwd)/AFLplusplus/afl-clang-lto++" \ | |||
-DBUILD_FOR_FUZZING=ON | |||
$ cmake --build build_fuzz -j$(nproc) | |||
$ cmake --build build_fuzz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried these, it's possible that these might also need a -no_warn_duplicate_libraries
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test it.
@@ -14,7 +14,7 @@ $ cmake -B build_fuzz \ | |||
-DSANITIZERS=undefined,address,fuzzer | |||
# macOS users: If you have problem with this step then make sure to read "macOS hints for | |||
# libFuzzer" on https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer | |||
$ cmake --build build_fuzz -j$(nproc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems to me we should likely update https://github.com/bitcoin/bitcoin/blob/19f0dd02c1373dd9d2241ce25cbbcb6831763725/doc/developer-notes.md?plain=1#L224 and https://github.com/bitcoin/bitcoin/blob/14093d5d243f6eb9cfef721c80f92848d95032ee/doc/productivity.md?plain=1#L63-L69 as well (both to cmake and the nproc reference)
@@ -150,6 +150,7 @@ $ cmake -B build_fuzz \ | |||
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \ | |||
-DBUILD_FOR_FUZZING=ON \ | |||
-DSANITIZERS=undefined,address,fuzzer \ | |||
-DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked, this works as well 👍
ACK 7c37119 |
A few amendments to the
fuzzing.md
:Also a "TODO" prefix has been added to the comment.