tests: Provide main(...) function in fuzzer. Allow building uninstrumented harnesses with --enable-fuzz. #19366

Provide main(...) function in fuzzer. Allow building uninstrumented harnesses with only --enable-fuzz.

This PR restores the behaviour to how things worked prior to #18008. #18008 worked around an macOS specific issue but did it in a way which unnecessarily affected platforms not in need of the workaround :)

Before this patch:

# Build uninstrumented fuzzing harness (no libFuzzer/AFL/other-fuzzer-instrumentation)
$ ./configure --enable-fuzz
$ make
  CXXLD    test/fuzz/span     
/usr/lib/gcc/x86_64-linux-gnu/7/../../../x86_64-linux-gnu/Scrt1.o: In function `_start':
(.text+0x20): undefined reference to `main'              
collect2: error: ld returned 1 exit status
Makefile:7244: recipe for target 'test/fuzz/span' failed                   
make[2]: *** [test/fuzz/span] Error 1
make[2]: *** Waiting for unfinished jobs....
$

After this patch:

# Build uninstrumented fuzzing harness (no libFuzzer/AFL/other-fuzzer-instrumentation)
$ ./configure --enable-fuzz
$ make
$ echo foo | src/test/fuzz/span
$

The examples above show the change in non-macOS functionality. macOS functionality is unaffected by this patch.

The wallet_basic.py --descriptors CI failure seems entirely unrelated :)

Reported as #19369.

@sipa @theuni I think this is in reply to the recent IRC conversation?

For reference:

[12:33] <sipa> MarcoFalke: i'm finding it a bit annoying that codebase changes often break existing fuzz tests, which require travis or an entirely separate build to find out
[12:33] <sipa> what do you think about "building" the fuzz tests in normal make all mode (but without the actual fuzzing enabled)
[12:34] <sipa> one caveat is that the fuzz tests currently already need c++17
[12:48] <bitcoin-git> [bitcoin] hebasto closed pull request [#18710](/bitcoin-bitcoin/18710/): Add local thread pool to CCheckQueue (master...200419-thread-pool) [#18710](/bitcoin-bitcoin/18710/)
[12:49] <bitcoin-git> [bitcoin] hebasto reopened pull request [#18710](/bitcoin-bitcoin/18710/): Add local thread pool to CCheckQueue (master...200419-thread-pool) [#18710](/bitcoin-bitcoin/18710/)
[13:11] <cfields> sipa / MarcoFalke: See top 2 commits here for how that could work: https://github.com/theuni/bitcoin/commits/build-fuzz
[13:57] <wumpus> sipa: I'm not sure it should be the default, but I do agree it should be easier (e.g. a single configure option) to build the fuzz tests as well sa everything else
[13:57] <wumpus> having to do a second build with either-or exclusive options is kind of a time sink and easy to forget
[13:58] <sipa> wumpus: it seems that building the actual fuzz tests within one configure is very hard
[13:59] <wumpus> okay
[13:59] <sipa> but just something that can test whether the fuzz tests build is great already, i think
[13:59] <sipa> because it's really easy to introduce compile errors in them (and this will likely worsen as their coverage increases)
[14:00] <wumpus> right
[14:01] <sipa> we could even have a minimal main() for the fake-fuzz tests that reads a single input from stdin, and runs the test code on it
[14:02] <sipa> or all entries in a directory... meaning it'd be actually useful as a test harness; you just wouldn't be able to fuzz with it
[14:06] <wumpus> that's a neat idea, at least the building feels less like dead weight in that case
[14:09] <sipa> as the travis fuzz build isn't actually doing any fuzzing, this may even be sufficient there
[14:09] <bitcoin-git> [bitcoin] jnewbery opened pull request [#19364](/bitcoin-bitcoin/19364/): net processing: Move orphan reprocessing to a global (master...2020-06-global-orphans) [#19364](/bitcoin-bitcoin/19364/)
[14:11] <cfields> it seems getting them to link as real progs will have linker issues due to missing libs. I guess the linker doesn't go looking for unresolved symbols with no main?
[14:11] <sipa> cfields: hmm, what symbols are missing?
[14:12] <cfields> sipa: zmq/miniupnpc/etc.
[14:12] <sipa> oh
[14:12] <sipa> that sounds fixable?
[14:12] <cfields> We could tack them on, but that kinda sucks.
[14:13] <cfields> Yeah, there's another approach though to avoid linking altogether though, 1 sec.
[14:13] <sipa> or maybe it's just not an issue
[14:13] <sipa> because if it's not a problem for real fuzzing, why would it here?
[14:13] <sipa> perhaps it's due to not having main() indeed
[14:17] <cfields> different approach: https://github.com/theuni/bitcoin/commit/d36b2b9cfe7d3916f261e88b63c03a7a3edd9ea5
[14:17] <cfields> That one's verbose though, requires us to c/p the object names to that list.
[14:20] <sipa> that's somewhat annoying (it's already annoying to need to add a blob to the makefile for every new fuzz test)
[14:20] <sipa> but not terrible i guess
[14:24] <cfields> I'll play with the linker errors a bit, see if there's a quick fix.
[14:49] <wumpus> even just compiling and not linking would find the largest part of the errors introduced by changes and drift i guess
[14:50] <wumpus> link-time errors are kind of rare

I'd say they should be compiled by default, just like bench and the unit tests, and the gui, and the gui tests. No one is running the benchmarks to do actual benchmarking in a usual development workflow. They are merely compiled to be smoke-run once in make check. I don't see why the fuzz tests should be different. They should be built and maybe even smoke tested (with the empty input or so) by default.

Similar to --disable-tests, --disable-gui-tests, --disable-bench, there should be a --disable-fuzz option.

The reason that fuzz tests are handled differently is because I am lacking the background in automake to do even the most simplest task. (I leave improvements in the build system for others to implement. E.g. #18527 :grimacing: )

Anyway, I think all the tests should be handled the same by the build system and make check.

I'd say they should be compiled by default, just like bench and the unit tests, and the gui, and the gui tests. No one is running the benchmarks to do actual benchmarking in a usual development workflow. They are merely compiled to be smoke-run once in make check. I don't see why the fuzz tests should be different. They should be built and maybe even smoke tested (with the empty input or so) by default.

Concept ACK. I think we'd want to skip building them in gitian (to save time for every platform, the result isn't shipped), but for a normal developer build it'd make sense.

Jup, I think gitian/guix set --disable-bench, so they could also disable the fuzz tests

@MarcoFalke I hadn't seen that conversation (I'm not on IRC), so this PR isn't a reply to it :)

I've simply needed this myself many times when using the fuzzing harnesses as simple application entry points for concolic testing tools, abstract interpretation static analysis tools, etc which analyze the possible execution flow from main and onwards :)

In other words, being able to build the fuzz targets (non-instrumented using --enable-fuzz) is of much value also outside of fuzzing :)

This PR is restoring how things worked under Linux prior to the "macOS libFuzzer linker issue" fixed in #18008 (I'm upstreaming a local fix I've had for this).

Regarding the quoted IRC conversation: I fully agree that we should start building the fuzzing harnesses by default, but I think that is largely unrelated to this PR.

FWIW, the coverage of the fuzzing harnesses is quickly approaching the coverage reached by the unit tests (see unit test coverage and fuzz test coverage). Hopefully writing fuzz tests to show a basic level of robustness for new code will soon be almost as natural as writing unit or functional tests for new code :) Building the fuzzers by default will hopefully help us get there even sooner.

review ACK fdb933cf68f777bb5e0aaabc6216c5b54b4b28f1

ACK 1087807b2bc56b9c7e7a5471c83f6ecfae79b048

Post-merge Concept ACK.

I'd say they should be compiled by default, just like bench and the unit tests, and the gui, and the gui tests.

Anyway, I think all the tests should be handled the same by the build system and make check.

I agree. This may catch a number of inadvertent fuzz build issues we've been seeing in PRs.