This fixes issue #19388. The changes are as follows:
Fixes #19388
Concept ACK on building fuzz tests by default, but needs squashing and fixing of failing CI jobs :)
Haven't reviewed any code or build changes.
375 | @@ -376,7 +376,7 @@ fi 376 | if test x$use_sanitizers != x; then 377 | dnl First check if the compiler accepts flags. If an incompatible pair like 378 | dnl -fsanitize=address,thread is used here, this check will fail. This will also 379 | - dnl fail if a bad argument is passed, e.g. -fsanitize=undfeined 380 | + dnl fail if a bad argument is passed, e.g. -fsanitize=undefined
Isn't this typo an example of a bad argument?
Hi - apologies for making you read the code in this state. Initially I had opened the PR because I had a patch that was working under Linux and hadn't yet discovered the CI tests. Since then I've been pushing sporadically as a sanity check against the CI test behavior on my own machine. I just learned about the "convert to draft" feature and applied it for the time being.
I'm not sure how those commits got duplicated or how someone else's commits got pulled in. I believe it happened the last time I was attempting to rebase onto master. In any case I do intend to clean everything up once I can get all the tests to pass.
I'll restore the typo; at the time I hadn't realized that "undefined" was a valid argument.
I'm confused by your comment about building under windows, so I think I'm missing some fundamental point about the original issue. I had expected that as part of making the change the fuzz tests would now be built by default in more contexts (ie Windows, and specifically as an example the win64 CI test) and any compilation errors would need to be fixed as part of it. If that's not true, then as alternatives I can imagine (a) explicitly not building under non-compiling environments regardless of the value of ENABLE_FUZZ, or (b) ignoring the fact that the CI tests break (if that is even possible), but neither seemed that desirable? Anyway please let me know what behavior you'd like and I'll make the change.
(I suppose a third option would be to require someone building under e.g. windows to specify --disable-fuzz)
I'd suggest splitting up this PR, because it seems like there are at least two separate changes here. i.e f1be9029e17ee702ddf19bbf6f08392ef4cdd8d6 "Allow building the fuzz tests under windows" shouldn't be related to whether or not we build the fuzz tests by default.
In any case, you need to rebase and fix up your branch, so that it only contains your changes. You can't have merge commits like 04f5799abfc683f91612fba41ba6e98bfa3b41a1. It looks like you've also got the same changes in multiple commits, i.e 622a2e7a5e47305574b04111ab8275f281158d12 and 0f2bdb1e26d262bb6d3d2950ea5ff81758a08f04.
Please don't close and open a new PR.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
The standard "formula" for a generic swap is:
...
{
using std::swap;
swap(a, b);
}
...
Not sure what the problem is, but maybe that's worth trying.
The standard "formula" for a generic swap is:
... { using std::swap; swap(a, b); } ...Not sure what the problem is, but maybe that's worth trying.
Thanks, I just hadn't looked carefully enough - it was a swap where one side was a vector location, and Element happened to be bool. Fingers crossed that this fixed it and I can clean up for review.
Well...almost there. I tried for a while to squash things down into a single commit, but I think Ive failed. The correct patch is at c7746f0 (except for the fact that I accidentally chopped off the first two lines of the commit message and now the linter is complaining), but I don't know how to get rid of the history (I used git rebase -i and manually squashed/dropped and this was the result). If someone could tell me how to fix this that would be much appreciated.
As far as the actual review, the substring match that I introduced feels kind of sketchy but I wasn't sure how else to accomplish what I wanted. The intent was to do some stuff only when libfuzzer was linked, which is what I thought the old check wanted to do as well but it still passed (that is, suppressed a main function etc) on e.g -fsanitize=thread. So I'm confused about what's going on there and would like feedback on that part in particular.
If you want to squash everything into one commit:
Reset the state to the latest commit that is in your branch, but not one of your changes. This is usually the latest commit from master that you merged in. In your case, it is 32b191fb66e644c690c94cbfdae6ddbc754769d7. Softly reset to that commit, so that all your changes are staged as one in the git staging area.
git reset --soft 32b191fb66e644c690c94cbfdae6ddbc754769d7
Now commit the staged changes with a message
git commit -m 'build: build fuzz tests by default'
push as usual ...
427 | @@ -428,8 +428,10 @@ class cache 428 | * for the next iteration. 429 | */ 430 | last_loc = locs[(1 + (std::find(locs.begin(), locs.end(), last_loc) - locs.begin())) & 7]; 431 | - std::swap(table[last_loc], e); 432 | // Can't std::swap a std::vector<bool>::reference and a bool&. 433 | + Element el = std::move(table[last_loc]); 434 | + table[last_loc] = std::move(e); 435 | + e = std::move(el);
I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?
Actually I'm a bit baffled by this one - the reason for this change was the instantiation of a bool cache in src/test/fuzz/cuckoocache.cpp, which looks like it would have been built whenever enable-fuzz was true. I don't understand how that ever compiled, but if you don't know the answer offhand I'll investigate.
All compilers are broken (as in: They compile wrong code without error, or conversely compile correct code into an incorrect binary). But we should document this brokenness as best as we can, and fix incorrect code separate from feature changes or refactoring or build system changes. Mixing everything into one commit/patchset will make it harder to review and bisect later on.
Code-wise, this looks pretty good, and in a short test it also seems to work.
I'm not entirely sure about the meaning of --{enable,disable}-fuzz that this introduces. There are really two distinct concepts now that are not exactly the same anymore:
So I think this leads to problems if you'd want to fuzz with AFL. You'd need to specify (or get default) --enable-fuzz, and not enable -fsanitize=fuzzer, resulting in a situation where all the non-fuzz make targets aren't disabled. I don't think this is a big issue, and I'm perfectly fine with getting this merged as is, and iterate later. However, I think the following alternative may actually be less invasive change:
--{enable,disable}-fuzz-binary option (default on) to indicate the need to build a ./src/test/fuzz/fuzz binary--{enable,disable}-fuzz as option (default off) to indicate a fuzzing build (which disables all other make targets, and enables --enable-fuzz-binary)
@sipa Based on your comment I wasn't sure if I should make the change you suggested or wait for someone else to weigh in, but I wanted to make sure that I'm not dropping the ball on anything.
I agree that your suggestion would be easy to implement. If we don't want to add another flag (perhaps desirable from a usability perspective?), I could also just disable non-fuzz targets when building with AFL or Honggfuzz.
1215 | @@ -1216,53 +1216,47 @@ dnl it would break GCC's #include_next. 1216 | AC_DEFUN([SUPPRESS_WARNINGS], 1217 | [$(echo $1 |${SED} -E -e 's/(^| )-I/\1-isystem /g' -e 's;-isystem /usr/include([/ ]|$);-I/usr/include\1;g')]) 1218 | 1219 | -dnl enable-fuzz should disable all other targets
On a second thought, this might be useful to keep. We don't want to waste resources compiling the wallet, util, cli, qt, bench, ... when the goal is to only fuzz.
Sorry, I'm confused by this. enable-fuzz refers to whether the fuzz targets should be built, which is now being done by default. It seems that we only want to disable the other targets when building with libfuzzer, afl or honggfuzz (the latter two of which still need to be addressed, which should be easy enough by inspecting CXX_FLAGS). Does this sound right to you?
Before your change enable-fuzz means build the fuzz targets, but disable all other targets After your change, it should keep that meaning, so that we don't waste compile resources. (I incorrectly assumed that it should go or change meaning).
After your change, the only thing that should change is that the fuzz target is always built (whether enable-fuzz or not).
There could be a disable-fuzz option that disables the fuzz target, but that can be done in a follow up. Unless you want to do it here right away.
@MarcoFalke I think we're giving @danben conflicting instructions here.
Did you see #20936 (comment)? I don't think we can just keep --{enable,disable}-fuzz with the same meaning.
I'm working on a patch that implement's @MarcoFalke's instructions as best I understand them just so that no matter how this gets resolved there will be a good place to begin wrapping things up.
The one thing that seemed a bit odd to me is this: if enable-fuzz used to mean "build the fuzz targets, but disable all other targets" and the expected change is that fuzz targets get built regardless of the value of enable-fuzz, then we end up with a flag whose name is enable-fuzz but all it does is disable building of non-fuzz targets.
Additionally, under these assumptions I don't think it's possible to punt on the flag that disables fuzz targets because without the ability to change code, some of the CI tests won't pass. But unfortunately this flag cannot be called disable-fuzz if we are saying that enable-fuzz has no effect on whether fuzz targets are built.
In my comment I assumed that enable-fuzz and disable-fuzz are separate concepts (I don't know if configure can treat them as such). I used different words to describe the same that @sipa did in the previous comment. I don't care about the names of the configure arguments, as long as they achieve what we want :)
71 | @@ -72,7 +72,11 @@ extern "C" int LLVMFuzzerInitialize(int* argc, char*** argv) 72 | } 73 | 74 | #if defined(PROVIDE_MAIN_FUNCTION) 75 | +#ifdef WIN32 76 | +int main(int args, char **argv) 77 | +#else 78 | __attribute__((weak)) int main(int argc, char** argv) 79 | +#endif
I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?
8 | @@ -9,7 +9,13 @@ 9 | 10 | #include <cassert> 11 | #include <cstdint> 12 | + 13 | +#ifdef WIN32 14 | +#include <winsock2.h> 15 | +#else 16 | #include <netinet/in.h> 17 | +#endif
I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?
77 | @@ -78,7 +78,9 @@ FUZZ_TARGET(string) 78 | } 79 | (void)SanitizeString(random_string_1); 80 | (void)SanitizeString(random_string_1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 3)); 81 | + #ifndef WIN32 82 | (void)ShellEscape(random_string_1); 83 | + #endif
I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?
275 | + v4_addr_tmp.S_un.S_addr = fuzzed_data_provider.ConsumeIntegral<uint32_t>(); 276 | + const in_addr v4_addr(v4_addr_tmp); 277 | + #else 278 | const in_addr v4_addr = { 279 | .s_addr = fuzzed_data_provider.ConsumeIntegral<uint32_t>()}; 280 | + #endif
I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?
So this should probably be --disable-fuzz-binary in the windows ci (instead of my suggestion --disable-fuzz, which might not go well with how configure is supposed to work)
I couldn't build a library with afl++, though the binaries did compile.
I expect this issue to "fix itself" once you address #20936 (review)
root@9d90aa3c2826:/bitcoin# make
Making all in src
make[1]: Entering directory '/bitcoin/src'
make[2]: Entering directory '/bitcoin/src'
make[3]: Entering directory '/bitcoin'
make[3]: Leaving directory '/bitcoin'
CXXLD libbitcoinconsensus.la
afl-cc ++3.01a by Michal Zalewski, Laszlo Szekeres, Marc Heuse - mode: LLVM-PCGUARD
Using unoptimized trace-pc-guard, upgrade to llvm 10.0.1+ for enhanced version.
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:80: multiple definition of `__afl_area_ptr'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:80: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:81: multiple definition of `__afl_area_ptr_backup'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:81: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:79: multiple definition of `__afl_area_ptr_dummy'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:79: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_auto_early':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1045: multiple definition of `__afl_auto_early'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1045: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_auto_first':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1088: multiple definition of `__afl_auto_first'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1088: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_auto_init':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1033: multiple definition of `__afl_auto_init'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1033: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_auto_second':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1057: multiple definition of `__afl_auto_second'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1057: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:(.tbss+0x24): multiple definition of `__afl_cmp_counter'; /AFLplusplus/afl-compiler-rt.o:(.tbss+0x24): first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_discard':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1422: multiple definition of `__afl_coverage_discard'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1422: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_interesting':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1452: multiple definition of `__afl_coverage_interesting'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1452: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_off':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1400: multiple definition of `__afl_coverage_off'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1400: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_on':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1412: multiple definition of `__afl_coverage_on'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1412: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_discard':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1432: multiple definition of `__afl_coverage_skip'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1432: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:85: multiple definition of `__afl_fuzz_len'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:85: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_manual_init':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1004: multiple definition of `__afl_manual_init'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1004: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:88: multiple definition of `__afl_map_size'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:88: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_persistent_loop':
llvm_mode/instrumentation/afl-compiler-rt.o.c:945: multiple definition of `__afl_persistent_loop'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:945: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:(.tbss+0x20): multiple definition of `__afl_prev_ctx'; /AFLplusplus/afl-compiler-rt.o:(.tbss+0x20): first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:(.tbss+0x0): multiple definition of `__afl_prev_loc'; /AFLplusplus/afl-compiler-rt.o:(.tbss+0x0): first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:95: multiple definition of `__afl_selective_coverage_temp'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:95: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_trace':
llvm_mode/instrumentation/afl-compiler-rt.o.c:135: multiple definition of `__afl_trace'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:135: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook1':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: multiple definition of `__cmplog_ins_hook1'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook2':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: multiple definition of `__cmplog_ins_hook2'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook4':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: multiple definition of `__cmplog_ins_hook4'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook8':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: multiple definition of `__cmplog_ins_hook8'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_rtn_hook':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1365: multiple definition of `__cmplog_rtn_hook'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1365: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook1':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: multiple definition of `__sanitizer_cov_trace_cmp1'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook2':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: multiple definition of `__sanitizer_cov_trace_cmp2'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook4':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: multiple definition of `__sanitizer_cov_trace_cmp4'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook8':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: multiple definition of `__sanitizer_cov_trace_cmp8'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook1':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: multiple definition of `__sanitizer_cov_trace_const_cmp1'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook2':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: multiple definition of `__sanitizer_cov_trace_const_cmp2'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook4':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: multiple definition of `__sanitizer_cov_trace_const_cmp4'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook8':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: multiple definition of `__sanitizer_cov_trace_const_cmp8'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__sanitizer_cov_trace_pc_guard':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1148: multiple definition of `__sanitizer_cov_trace_pc_guard'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1148: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__sanitizer_cov_trace_pc_guard_init':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1163: multiple definition of `__sanitizer_cov_trace_pc_guard_init'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1163: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__sanitizer_cov_trace_switch':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1326: multiple definition of `__sanitizer_cov_trace_switch'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1326: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `at_exit':
llvm_mode/instrumentation/afl-compiler-rt.o.c:129: multiple definition of `at_exit'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:129: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `send_forkserver_error':
llvm_mode/instrumentation/afl-compiler-rt.o.c:162: multiple definition of `send_forkserver_error'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:162: first defined here
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [Makefile:5482: libbitcoinconsensus.la] Error 1
make[2]: Leaving directory '/bitcoin/src'
make[1]: *** [Makefile:15061: all-recursive] Error 1
make[1]: Leaving directory '/bitcoin/src'
make: *** [Makefile:809: all-recursive] Error 1
Turned review back on. Two things to note:
- I have no idea what this appveyor failure is trying to tell me. It involves a file that I didn't touch.
Rebase on top of master and appveyor will be happily green again now that #21051 has been merged :)
1226 | @@ -1221,6 +1227,14 @@ dnl it would break GCC's #include_next. 1227 | AC_DEFUN([SUPPRESS_WARNINGS], 1228 | [$(echo $1 |${SED} -E -e 's/(^| )-I/\1-isystem /g' -e 's;-isystem /usr/include([/ ]|$);-I/usr/include\1;g')]) 1229 | 1230 | +AC_MSG_CHECKING([whether test/fuzz/fuzz main function is needed]) 1231 | +if [[[ $use_sanitizers == *"fuzzer"* ]]]; then
I am pretty sure this will break honggfuzz. Is there anything wrong with how this was checked previously?
Ok, once you rebase I expect honggfuzz to be broken
Concept ACK. Looks better now, except that it breaks honggfuzz
@MarcoFalke Prior to reopening I built and ran honggfuzz (as per doc/fuzzing.md) with no issue. Perhaps it's something to do with my system; how does it break for you?
Re: your question above, I had changed it because (for reasons that I still don't quite understand) the check would pass for me even when specifying e.g. --fsanitize=thread which caused several CI tests to fail now that the fuzz targets are built. That said, I would have expected the old check to do the right thing so I'm fairly confused by this.
Oh, sorry. I was testing master :man_facepalming: , which means honggfuzz is broken in master and fixed in this pull? I'll need to test some more...
Edit: Broken on master and this pr.
That said, I would have expected the old check to do the right thing so I'm fairly confused by this.
I am incredibly confused by this as well. Tried to fix in #21080
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
I think what you want now is:
if ENABLE_FUZZ:
* Run MAIN_FUNCTION configure check
else:
* PROVIDE_MAIN_FUNCTION=yes
fi
I think what you want now is:
if ENABLE_FUZZ: * Run MAIN_FUNCTION configure check else: * PROVIDE_MAIN_FUNCTION=yes fi
Yep, got it changed locally and just testing everything now. Should be ready soon.
@MarcoFalke Strangely, now I'm seeing the same issue as in #19633. I reinstalled boost and tried a fresh build but it did not help. Pushing blindly for now.
1263 | - #include <cstddef> 1264 | - extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { return 0; } 1265 | - /* comment to remove the main function ... 1266 | - ]],[[ 1267 | - */ int not_main() { 1268 | - ]])])
Are the whitespace changes needed?
ACK on this commit if the ci is green. Will test tomorrow
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
This fixes issue #19388. The changes are as follows:
- Add a new flag to configure, --enable-fuzz-binary, which allows building test/fuzz/fuzz regardless of whether we are building to do actual fuzzing
- Set -DPROVIDE_MAIN_FUNCTION whenever --enable-fuzz is no
- Add the following libraries to FUZZ_SUITE_LD_COMMON:
- LIBBITCOIN_WALLET
- SQLLITE_LIBS
- BDB_LIBS
- if necessary, some or all of:
- NATPMP_LIBS
- MINIUPNPC_LIBS
- LIBBITCOIN_ZMQ / ZMQ_LIBSreview ACK 32cbb06676e2957705d3ca7ff5710c9c3ff22c14 📭
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK 32cbb06676e2957705d3ca7ff5710c9c3ff22c14 📭
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjAcQv/UflCiheoLDkcJSLBL3bLz9eoDY4DFaXXRstCZAmqnb4jdQYbW/uW60J4
B6Xhyx9mewauy+uKaspc+KOKVk3OSmGfnMa0ibIUuZ8w+SPUgKN/O2UeDfLEjqG5
e/wuYbOPNc9JI6xiPTRI6XRCtUnWLSPK4x9lymqbUS3lNtBQ+JupW+fxTQK1+lC2
xb//K9l9wnMoq8ZyOmfVbprzrDgyfSpEo3jTdeVlo5FraE7WQrgCSR0uix2QxZfp
DX7FOJrG3mld8PhR4AMQ0pZgJv4fDUn3fqMEg36/Aaz/SRUXpSCAZdZ1vNU3xKjv
CyyPmLAaAhDfIrKyPy7KLee6VpeczM4klMdWKI8bzVLlPs8kUGJPvc9uaaE7CUua
ValfGAXpN8tzdhmss8qvQyd50SOjnBhhoRcpVTVfSMttEoYOLXoMSfHhmMK7JC9h
YPqXcER367uTsINlUq3yMxJkYK6kkT0D/ki6doTjppy9YAzFCZin2zPpj/X6Zvgk
9O61+Nfq
=eqEg
-----END PGP SIGNATURE-----
Timestamp of file with hash 6ff121421f769a4f3402980038ef6e89d55175dfce07728a3f9adfac23f10030 -
</details>
187 | @@ -172,12 +188,12 @@ test_test_bitcoin_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) $( 188 | 189 | if ENABLE_ZMQ 190 | test_test_bitcoin_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS) 191 | +FUZZ_SUITE_LD_COMMON += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS) 192 | endif 193 | 194 | -if ENABLE_FUZZ 195 | - 196 | FUZZ_SUITE_LDFLAGS_COMMON = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) $(PTHREAD_FLAGS)
unrelated style nit: This is only used once, so you can inline it
1277 | @@ -1271,6 +1278,8 @@ else 1278 | QT_DBUS_INCLUDES=SUPPRESS_WARNINGS($QT_DBUS_INCLUDES) 1279 | QT_TEST_INCLUDES=SUPPRESS_WARNINGS($QT_TEST_INCLUDES) 1280 | fi 1281 | + 1282 | + CPPFLAGS="$CPPFLAGS -DPROVIDE_MAIN_FUNCTION"
unrelated style nit: This can be called DPROVIDE_FUZZ_MAIN_FUNCTION for clarity
Upgrade to Tested ACK for libfuzzer and honggfuzz
15 | @@ -16,4 +16,5 @@ export RUN_UNIT_TESTS_SEQUENTIAL="true" 16 | export RUN_UNIT_TESTS="false" 17 | export GOAL="install" 18 | export PREVIOUS_RELEASES_TO_DOWNLOAD="v0.15.2 v0.16.3 v0.17.2 v0.18.1 v0.19.1" 19 | -export BITCOIN_CONFIG="--enable-zmq --with-libs=no --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports --enable-debug CFLAGS=\"-g0 -O2 -funsigned-char\" CXXFLAGS=\"-g0 -O2 -funsigned-char\" --with-boost-process" 20 | +export BITCOIN_CONFIG="--enable-zmq --with-libs=no --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports 21 | +--enable-debug --disable-fuzz-binary CFLAGS=\"-g0 -O2 -funsigned-char\" CXXFLAGS=\"-g0 -O2 -funsigned-char\" --with-boost-process"
feel free to follow up with pull requests to remove the temporary disable-fuzz-binary
12 | @@ -13,7 +13,7 @@ export PACKAGES="python3 nsis g++-mingw-w64-x86-64 wine-binfmt wine64 file" 13 | export RUN_FUNCTIONAL_TESTS=false 14 | export RUN_SECURITY_TESTS="true" 15 | export GOAL="deploy" 16 | -export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests --without-boost-process" 17 | +export BITCOIN_CONFIG="--enable-reduce-exports --disable-fuzz-binary --disable-gui-tests --without-boost-process"
same
left some ideas for follow-ups
