[llvm] r310075 - Port libFuzzer tests to LIT. Do not require two-stage build for check-fuzzer.

[George Karpenkov via llvm-commits]

> On Aug 17, 2017, at 4:24 PM, Justin Bogner <mail at justinbogner.com> wrote:
> 
> George Karpenkov via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> Author: george.karpenkov
>> Date: Fri Aug  4 10:19:45 2017
>> New Revision: 310075
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=310075&view=rev
>> Log:
>> Port libFuzzer tests to LIT. Do not require two-stage build for check-fuzzer.
> 
> While the idea behind this change is reasonable, this way of doing it
> introduces some serious problems. Notably, building LLVM and clang's
> fuzzers now uses an instrumented libFuzzer,

I think the particular problem you are referencing was fixed in https://github.com/llvm-mirror/llvm/commit/79302ff8ea636c2656f7fe34f299ca808109987f

> and you can no longer test
> libFuzzer at all unless you build clang in the same tree.

Right, but that holds true for all sanitizers.
In which scenario is that a problem? You need a freshly built compiler anyway.

> 
>> This revision ports all libFuzzer tests apart from the unittest to LIT.
>> The advantages of doing so include:
>> 
>> - Tests being self-contained
>> - Much easier debugging of a single test
>> - No need for using a two-stage compilation
>> 
>> The unit-test is still compiled using CMake, but it does not need a
>> freshly built compiler.
> 
> Yes it does - you're just relying on building that freshly built
> compiler first. More details below.

I’m not sure it does — why?

> 
>> NOTE: The previous two-stage bot configuration will NOT work, as in the
>> second stage build LLVM_USE_SANITIZER is set, which disables ASAN from
>> being built.
>> Thus bots will be reconfigured in the next few commits.
>> 
>> Differential Revision: https://reviews.llvm.org/D36295
> ...
>> Modified: llvm/trunk/lib/Fuzzer/CMakeLists.txt
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Fuzzer/CMakeLists.txt?rev=310075&r1=310074&r2=310075&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Fuzzer/CMakeLists.txt (original)
>> +++ llvm/trunk/lib/Fuzzer/CMakeLists.txt Fri Aug  4 10:19:45 2017
>> @@ -13,22 +13,17 @@ if( APPLE )
>>   endif()
>> endif()
>> 
>> -set(LIBFUZZER_FLAGS_BASE "${CMAKE_CXX_FLAGS}")
>> -if( LLVM_USE_SANITIZE_COVERAGE )
>> -  if(NOT "${LLVM_USE_SANITIZER}" STREQUAL "Address")
>> -    message(FATAL_ERROR
>> -      "LibFuzzer and its tests require LLVM_USE_SANITIZER=Address and "
>> -      "LLVM_USE_SANITIZE_COVERAGE=YES to be set."
>> -      )
>> -  endif()
>> -
>> -  # Disable the coverage and sanitizer instrumentation for the fuzzer itself.
>> -  set(CMAKE_CXX_FLAGS "${LIBFUZZER_FLAGS_BASE} -fno-sanitize-coverage=trace-pc-guard,edge,trace-cmp,indirect-calls,8bit-counters -Werror")
> 
> This makes it so that when we build LLVM with sanitizer coverage (for
> example when building llvm fuzzer tools) we also build libFuzzer with
> coverage, but that doesn't actually make sense.

Again, I think this was fixed in https://github.com/llvm-mirror/llvm/commit/79302ff8ea636c2656f7fe34f299ca808109987f

> We really do want to be
> disabling sanitizer instrumentation for libFuzzer when building the in
> tree fuzzers, otherwise we take a large performance hit and the fuzzer
> spends time exploring paths in the fuzzing logic itself.

Of course.

> This looks like it might be the cause of some significant recent slow
> down in an experimental ISel fuzzer bot I've been running.

Can you provide more details? libFuzzer itself should not be instrumented, otherwise it’s a bug.

> 
>> Modified: llvm/trunk/lib/Fuzzer/test/CMakeLists.txt
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Fuzzer/test/CMakeLists.txt?rev=310075&r1=310074&r2=310075&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Fuzzer/test/CMakeLists.txt (original)
>> +++ llvm/trunk/lib/Fuzzer/test/CMakeLists.txt Fri Aug  4 10:19:45 2017
> ...
>> @@ -257,11 +67,23 @@ if (MSVC)
>>   set(LIBFUZZER_POSIX 0)
>> endif()
>> 
>> +# Use just-built Clang to compile/link tests on all platforms, except for
>> +# Windows where we need to use clang-cl instead.
>> +if(NOT MSVC)
>> +  set(LIBFUZZER_TEST_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang)
>> +  set(LIBFUZZER_TEST_CXX_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++)
>> +else()
>> +  set(LIBFUZZER_TEST_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang.exe)
>> +  set(LIBFUZZER_TEST_CXX_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++.exe)
>> +endif()
> 
> What about when we just have an LLVM checkout and we're not building
> clang? As implemented, this patch makes it impossible to test libFuzzer
> with a clang that you've already built, and ends up spewing strange
> errors if you try to test libFuzzer without a clang checkout. We need
> some way to say "use the host compiler”.

But the previous way of testing libFuzzer was even more awkward: you had to build all of LLVM with ENABLE_SANITIZER_COVERAGE
(or a similarly named flag).

Having said that, it would be quite easy to introduce a CMake variable which would let the user override the compiler.
Is that what you wish? That should be only a one-line change.