[PATCH] D20558: [LibFuzzer] Start cleaning up the CMakeLists.txt files.

Tue May 24 11:27:58 PDT 2016

delcypher added a comment.

In http://reviews.llvm.org/D20558#437510, @kcc wrote:

> In http://reviews.llvm.org/D20558#437483, @delcypher wrote:
>
> > @kcc : This probably isn't ready to go quite yet as I have some questions whose answers will change this patch.
> >
> > 1. How is CMake supposed to be configured when the intention is to build libFuzzer and its tests? In the end I found doing
>
>
> Why did you have to "find" it? 
>  It's documented: http://llvm.org/docs/LibFuzzer.html#fuzzing-components-of-llvm

Ah sorry. I didn't look at that part of the documentation because I wasn't trying to build any of the LLVM specific fuzzers.

> 

> 

> >   CC=/path/to/recent/clang CXX=/path/to/recent/clang++ cmake -DLLVM_USE_SANITIZE_COVERAGE=ON -DLLVM_USE_SANITIZER=Address /path/to/llvm/src/root

> 

> > 

> 

> > 

> 

> > seemed to work and the tests would pass under Linux with this patch. Is that the intended way of configuring/building?

> 

> > 

> 

> > 2. It is possible to build libFuzzer and the tests **without** `-DLLVM_USE_SANITIZER=Address` being passed to CMake but when I do that the additional flags `-fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp` (from `HandleLLVMOptions.cmake`) don't get given put in `CMAKE_CXX_FLAGS`. It looks like the previous CMake code implicitly assumed those flags were set. This cause a great deal of confusion for me as I tried configuring without `-DLLVM_USE_SANITIZER=Address` when writing the patch hence some of the `FIXME`s. The reason it's so confusing is that in some places `-fsanitize-coverage=` gets explicitly set (i.e in `/lib/Fuzzer/CMakeLists.txt`) but in other places it is implicitly assumed that `-fsanitize-coverage=` has been set.

> 

> 

> -fsanitize-coverage flags are allowed only in combination with one of the sanitizers. 

>  besides, some of the libfuzzer tests actually expect asan

> 

> > This patch hasn't tried to fix this but I think we need to. We either need to disallow building libFuzzer without `LLVM_USE_SANITIZER` being set appropriately (not my preferred approach), or we need to make setting the `-fsanitize-coverage=` flag always explicit in the LibFuzzer and tests `CMakeLists.txt` files.

> 

> > 

> 

> > Another thing worth considering is that without `-fsanitize=address` using `-fsanitize-coverage=` does absolutely nothing and clang just emits a warning that the flag is unused. This won't cause a build failure but the tests will obviously fail.

> 

> 

> Yes, that's intentional.

> 

> > Considering the above may I should do both, i.e. be explicit about what `-fsanitize-coverage=` is being set to and also deny building LibFuzzer without a sanitizer being enabled. Thoughts?

> 

> 

> Dunno. I like the current way of doing things.

>  Remind me, what problem are you trying to solve? (Other than allowing the non-Release build with libFuzzer)?

There are two additional problems I'd like to solve

1. Building the libFuzzer tests shouldn't be possible if CMake hasn't been configured with `LLVM_USE_SANITIZER`
2. I'd like the setting of the `-fsanitize-coverage=` flags to be more explicit.

Now that I've had sometime to think about it for problem '1' I've decided I'm going to modify the patch to check for this.

For problem '2' I think it would be helpful for me to explain how things work now:

For tests declared in `lib/Fuzzer/tests/CMakeLists.txt`
-------------------------------------------------------

`-fsanitize-coverage=edge,indirect-calls` is directly applied to the tests executables declared in `lib/Fuzzer/tests/CMakeLists.txt`. However there is already a `-fsanitize-coverage=` flag in `CMAKE_CXX_FLAGS` (that is set in a completely different place in the build system i.e. `cmake/modules/HandleLLVMOptions.cmake`) so the command line looks like

  clang++ ... -fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp  ... -fsanitize-coverage=edge,indirect-calls ... 

Notice how `-fsanitize-coverage=` is specified twice. I'm guessing the right most  `-fsanitize-coverage=` argument is what is used but doing this is not good for clarity.

For tests declared in `lib/Fuzzer/test/dfsan/CMakeLists.txt`
------------------------------------------------------------

The tests executables declared in `lib/Fuzzer/test/dfsan/CMakeLists.txt` uses `fsanitize=address -fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp` which comes from `cmake/modules/HandleLLVMOptions.cmake` setting CMAKE_CXX_FLAGS.

For tests declared in  `lib/Fuzer/test/trace-bb/CMakeLists.txt`
---------------------------------------------------------------

The test executables declared in `lib/Fuzer/test/trace-bb/CMakeLists.txt` directly specify `-fsanitize-coverage=edge,trace-bb`  but also get a `-fsanitize-coverage=` argument from `cmake/modules/HandleLLVMOptions.cmake` so the command line is like

  clang++ ... -fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp ...-fsanitize-coverage=edge,trace-bb ...

so `-fsanitize-coverage=` is specified twice.

For tests declared in  `lib/Fuzer/test/trace-pc/CMakeLists.txt`
---------------------------------------------------------------

The test executables declared in `lib/Fuzer/test/trace-pc/CMakeLists.txt` use `-fno-sanitize-coverage=8bit-counters -fsanitize-coverage=trace-pc` which is declared directly but also get a `-fsanitize-coverage=` argument from `cmake/modules/HandleLLVMOptions.cmake` so the command line is like

  clang++  ... -fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp  ...  -fno-sanitize-coverage=8bit-counters -fsanitize-coverage=trace-pc

For tests declared in `lib/Fuzzer/test/ubsan/CMakeLists.txt`
------------------------------------------------------------

The test executables declared in `lib/Fuzzer/test/ubsan/CMakeLists.txt` use `fsanitize=address -fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp` which comes from `cmake/modules/HandleLLVMOptions.cmake` setting CMAKE_CXX_FLAGS.

For tests declared in `lib/Fuzzer/test/uninstrumented/CMakeLists.txt`
---------------------------------------------------------------------

- The test executables declared in `lib/Fuzzer/test/uninstrumented/CMakeLists.txt` use `-fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters`  but also get a `-fsanitize-coverage=` argument from `cmake/modules/HandleLLVMOptions.cmake` so the command line is like

  clang++ ... -fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp ... -fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters ...

Why is this confusing?
----------------------

The implicit presence of `-fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp` is not at all obvious when reading the `CMakeLists.txt` files because `cmake/modules/HandleLLVMOptions.cmake` is very far away. Also specifying flags multiple times (although understandable) is unnecessarily complicated.

The best way to fix this is to remove the `-fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp` from CMAKE_CXX_FLAGS when building libFuzzer and its tests and then explicitly set `-fsanitize-coverage=` to what it needs to be.

http://reviews.llvm.org/D20558