[libFuzzer] Diff 44 - Remove dependency for tests on Windows.

[mpividori]

Summary

Remove dependency on FileCheck and not for tests on Windows.
If LLVM_USE_SANITIZER=Address and LLVM_USE_SANITIZE_COVERAGE=YES, this will trigger the building of target FileCheck with sanitizer instrumentation. This will fail in Windows, since cmake will use link.exe for linking and won't include compiler-rt libraries.

[kcc]

I'd love to get rid of these extra cmake flags, but I am not sure this is the right time.
Now, we also require that the host compiler is a fresh clang (preferably, from the same revision) and we have no way to enforce that.
So these pesky cmake flags "kind of" protect us accidentally using a non-fresh-clang host compiler (because they force the user to read the docs)

lib/Fuzzer/test/CMakeLists.txt

35 ↗

(On Diff #82122)

Why not add -fsanitize=address unconditionally?

[mpividori]

@kcc Ok. So, I see two options to achieve both goals:
+ Force the user to read the docs.
+ Make it work on Windows (don't use instrumentation for FileCheck target).

Or remove these flags, (as this diff does), and add a new cmake flag: ENABLE_LIBFUZZER , with default value false.
Or modify ./modules/HandleLLVMOptions.cmake, to not append "-fsanitize-coverage=trace-pc-guard,indirect-calls,trace-cmp" when LLVM_USE_SANITIZE_COVERAGE is true.

lib/Fuzzer/test/CMakeLists.txt

35 ↗

(On Diff #82122)

@kcc Yes, I can do that. I just wanted to avoid duplicating a flag when it is not necessary.

[kcc]

In D27993#631795, @mpividori wrote:

@kcc Ok. So, I see two options to achieve both goals:
+ Force the user to read the docs.

Never works.

+ Make it work on Windows (don't use instrumentation for FileCheck target).

One option that you proposed off-line is to remove dependency on FileCheck (win-only) and rely on having it in PATH.
Hacky, but simple.

Or remove these flags, (as this diff does), and add a new cmake flag: ENABLE_LIBFUZZER , with default value false.

Mmmm. What will it change?

Or modify ./modules/HandleLLVMOptions.cmake, to not append "-fsanitize-coverage=trace-pc-guard,indirect-calls,trace-cmp" when LLVM_USE_SANITIZE_COVERAGE is true.

The purpose of LLVM_USE_SANITIZE_COVERAGE is to build the rest of llvm with sanitizer coverage so that we can build things like tools/clang/tools/clang-fuzzer

All this means that we can't bootstrap llvm with asan on windows, right?
I wonder if someone wants to solve this problem too...

[mpividori]

In D27993#631931, @kcc wrote:

In D27993#631795, @mpividori wrote:

@kcc Ok. So, I see two options to achieve both goals:
+ Force the user to read the docs.

Never works.

+ Make it work on Windows (don't use instrumentation for FileCheck target).

One option that you proposed off-line is to remove dependency on FileCheck (win-only) and rely on having it in PATH.
Hacky, but simple.

Or remove these flags, (as this diff does), and add a new cmake flag: ENABLE_LIBFUZZER , with default value false.

Mmmm. What will it change?

By default cmake won't build libFuzzer. So, this will achieve your original intention, users need to read the documentation to build libFuzzer.
Also, with the advantage that we don't need LLVM_USE_SANITIZE_COVERAGE, so we avoid including instrumentation in all the repository, which is not required for libFuzzer.
So, using ENABLE_LIBFUZZER, it will work for both, Linux and Windows.
I think this is a good solution, since we need a specific flag to enable / disable libFuzzer, independent of LLVM_USE_SANITIZE_COVERAGE.

[kcc]

In D27993#631945, @mpividori wrote:

In D27993#631931, @kcc wrote:

In D27993#631795, @mpividori wrote:

@kcc Ok. So, I see two options to achieve both goals:
+ Force the user to read the docs.

Never works.

+ Make it work on Windows (don't use instrumentation for FileCheck target).

One option that you proposed off-line is to remove dependency on FileCheck (win-only) and rely on having it in PATH.
Hacky, but simple.

Or remove these flags, (as this diff does), and add a new cmake flag: ENABLE_LIBFUZZER , with default value false.

Mmmm. What will it change?

By default cmake won't build libFuzzer. So, this will achieve your original intention, users need to read the documentation to build libFuzzer.
Also, with the advantage that we don't need LLVM_USE_SANITIZE_COVERAGE, so we avoid including instrumentation in all the repository, which is not required for libFuzzer.
So, using ENABLE_LIBFUZZER, it will work for both, Linux and Windows.
I think this is a good solution, since we need a specific flag to enable / disable libFuzzer, independent of LLVM_USE_SANITIZE_COVERAGE.

But this will not solve the problem of building the fuzzers in the llvm tree (like clang-fuzzer)

[mpividori]

@kcc We can continue using LLVM_USE_SANITIZE_COVERAGE for the rest of the repository, when we need instrumentation in all the code.
But for building libFuzzer, we don't need that, so I think it makes sense to use a different flag to enable/disable libFuzzer.
So, I propose to use ENABLE_LIBFUZZER in this Cmake file, instead of LLVM_USE_SANITIZER.
Would you agree?
Thanks,

[kcc]

In D27993#636374, @mpividori wrote:

@kcc We can continue using LLVM_USE_SANITIZE_COVERAGE for the rest of the repository, when we need instrumentation in all the code.
But for building libFuzzer, we don't need that, so I think it makes sense to use a different flag to enable/disable libFuzzer.
So, I propose to use ENABLE_LIBFUZZER in this Cmake file, instead of LLVM_USE_SANITIZER.
Would you agree?
Thanks,

It means one more non-default build config... No, I'd prefer to avoid it.

[mpividori]

@kcc What do you mean by "non-default build config"? In a previous comment you explained that you use this flag to protect from accidentally using a non freash clang host compiler, so this would be the purpose of ENABLE_LIBFUZZER
It could be removed when starting using monorepo.

[kcc]

In D27993#636392, @mpividori wrote:

@kcc What do you mean by "non-default build config"? In a previous comment you explained that you use this flag to protect from accidentally using a non freash clang host compiler, so this would be the purpose of ENABLE_LIBFUZZER
It could be removed when starting using monorepo.

Right now if you want to build clang-fuzzer you need to use a non-standard build config.
Same for libFuzzer tests, but at least it's the same build config.
You propose to have two different non-standard build configs.

[mpividori]

@kcc exactly, because these are different build config:
+ The purpose of LLVM_USE_SANITIZE_COVERAGE is to active the coverage flags for all the repository.
+ The purpose of ENABLE_LIBFUZZER is to avoid automatically building libFuzzer with a non fresh clang compiler, which would fail.
Also, I could automatical set ENABLE_LIBFUZZER=true when LLVM_USE_SANITIZE_COVERAGE=true.

Anyway, the main intention of these changes is to make it work on Windows. If you prefer, I can continue using only LLVM_USE_SANITIZE_COVERAGE and remove dependency on FileCheck (win-only) and rely on having it in PATH.

[kcc]

In D27993#636403, @mpividori wrote:

@kcc exactly, because these are different build config:
+ The purpose of LLVM_USE_SANITIZE_COVERAGE is to active the coverage flags for all the repository.
+ The purpose of ENABLE_LIBFUZZER is to avoid automatically building libFuzzer with a non fresh clang compiler, which would fail.
Also, I could automatical set ENABLE_LIBFUZZER=true when LLVM_USE_SANITIZE_COVERAGE=true.

Anyway, the main intention of these changes is to make it work on Windows. If you prefer, I can continue using only LLVM_USE_SANITIZE_COVERAGE and remove dependency on FileCheck (win-only) and rely on having it in PATH.

Yes, that would be a simpler med-term solution.

In D27993#636403, @mpividori wrote:

@kcc exactly, because these are different build config:
+ The purpose of LLVM_USE_SANITIZE_COVERAGE is to active the coverage flags for all the repository.
+ The purpose of ENABLE_LIBFUZZER is to avoid automatically building libFuzzer with a non fresh clang compiler, which would fail.
Also, I could automatical set ENABLE_LIBFUZZER=true when LLVM_USE_SANITIZE_COVERAGE=true.

Anyway, the main intention of these changes is to make it work on Windows. If you prefer, I can continue using only LLVM_USE_SANITIZE_COVERAGE and remove dependency on FileCheck (win-only) and rely on having it in PATH.

[kcc]

LGTM

llvm/trunk/lib/Fuzzer/test/CMakeLists.txt

 
 configure_lit_site_cfg(
   ${CMAKE_CURRENT_SOURCE_DIR}/unit/lit.site.cfg.in
   ${CMAKE_CURRENT_BINARY_DIR}/unit/lit.site.cfg
   )
 
 add_lit_testsuite(check-fuzzer "Running Fuzzer tests"
     ${CMAKE_CURRENT_BINARY_DIR}
-    DEPENDS ${TestBinaries} FileCheck sancov not
     )
+
+# Don't add dependencies on Windows. The linker step would fail on Windows,
+# since cmake will use link.exe for linking and won't include compiler-rt libs.
+if(NOT MSVC)
+  add_dependencies(check-fuzzer FileCheck sancov not)
+endif()