-
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: Add fuzzing facilities #43
Conversation
You might be interested in this PR :) |
I'm pretty sure we will need the equivalent of |
1a00d05
to
7dc3874
Compare
21ef1ab
to
2f63087
Compare
Rebased. |
d3f3967
to
7400ca5
Compare
424296e
to
7f7a472
Compare
Rebased and updated. |
631ff31
to
0cc0f9b
Compare
6174485
to
4291baa
Compare
0cc0f9b
to
ee718ad
Compare
Rebased. |
399642a
to
ecf10f1
Compare
ee718ad
to
073b93f
Compare
Rebased. |
ecf10f1
to
2af94fb
Compare
073b93f
to
7d33a75
Compare
Rebased. |
d2e741e
to
1958b54
Compare
7d33a75
to
7504618
Compare
Ping @fanquake re sanitizers. I'm curious if you think we should drop support for forwarding those flags as well. |
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.
7504618
to
910fd7a
Compare
This PR is ready for review now. Friendly ping @dergoegge @maflcko @fanquake @TheCharlatan @theuni :) |
@@ -72,6 +72,26 @@ tristate_option(WITH_USDT | |||
|
|||
option(BUILD_TESTS "Build test_bitcoin executable." ON) | |||
option(BUILD_BENCH "Build bench_bitcoin executable." ON) | |||
cmake_dependent_option(BUILD_FUZZ_BINARY "Build fuzz binary." ON "NOT MSVC" OFF) |
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.
Note to other reviewers because I had to look up this syntax again. This does the obvious thing: If not msvc, provide the BUILD_FUZZ_BINARY
option and set it to ON by default.
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.
@@ -72,6 +72,26 @@ tristate_option(WITH_USDT | |||
|
|||
option(BUILD_TESTS "Build test_bitcoin executable." ON) | |||
option(BUILD_BENCH "Build bench_bitcoin executable." ON) | |||
cmake_dependent_option(BUILD_FUZZ_BINARY "Build fuzz binary." ON "NOT MSVC" OFF) | |||
cmake_dependent_option(FUZZ "Build for fuzzing. Enabling this will disable all other targets and override BUILD_FUZZ_BINARY." OFF "NOT MSVC" OFF) |
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.
And this one means: If not msvc, provide the FUZZ option and set it to OFF by default.
910fd7a
to
c06f421
Compare
Rebased. |
Tested the oss-fuzz builds for libfuzzer and afl++, looking good! |
67ceebb
to
203a3ab
Compare
c06f421
to
e3536de
Compare
Rebased. |
e3536de
to
86bca52
Compare
As it was requested during yesterday's meeting, the google/oss-fuzz#11516 has been submitted on testing purposes. UPD. See hebasto/oss-fuzz#1. |
86bca52
to
9b9f562
Compare
https://github.com/hebasto/oss-fuzz/actions/runs/7585104640 is 🟢 :) |
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.
Lightly tested ACK 9b9f562.
I went through the deps to see if anything could be scoped more minimally, but it seems what's here is correct.
63c1bb5 fixup! cmake: Add fuzzing options (Hennadii Stepanov) d31abc2 fixup! ci: Test CMake edge cases (Hennadii Stepanov) Pull request description: From #85 (comment): > **Known Bugs** > > Unfortunately, due to a silent conflict between #43 and #77, providing `-DFUZZ=ON` does not disable the `bitcoin-qt` target. It will be fixed shortly after pushing this branch. Fixed. ACKs for top commit: vasild: ACK 63c1bb5 Tree-SHA512: b3cc2889d0239913de64c170880c97b37966a890d8c4e05f9090485a016b7f9cdf4880d770a234f323d3191b9adda8ed0343c29dfa49b5bb99b0b54481d4335e
I think we forgot to update the fuzzing docs https://github.com/hebasto/bitcoin/blob/cmake-staging/doc/fuzzing.md |
New CMake variables that affect the build system configuration:
SANITIZERS
BUILD_FUZZ_BINARY
FUZZ
In the light of bitcoin#29189, this PR is no longer based on #41. However, the
test/fuzz/script_bitcoin_consensus.cpp
might be easily added anytime.For OSS-Fuzz integration, please refer to hebasto/oss-fuzz#1.