This is an archive of the discontinued LLVM Phabricator instance.

[LibFuzzer] Allow LibFuzzer to be built in modes other than RELEASE.
ClosedPublic

Authored by delcypher on May 23 2016, 9:39 PM.

Details

Summary

[LibFuzzer] Allow LibFuzzer to be built in modes other than RELEASE.

Previously the flags were only being set correctly when the
build type was "Release". Now the build should work properly
for all the supported build types. When building libFuzzer
the optimization level respects whatever is used for the
rest of LLVM but for the LibFuzzer tests we force -O0.

Diff Detail

Event Timeline

delcypher updated this revision to Diff 58193.May 23 2016, 9:39 PM
delcypher retitled this revision from to [LibFuzzer] Start cleaning up the CMakeLists.txt files..
delcypher updated this object.
delcypher added a reviewer: kcc.
delcypher added a subscriber: llvm-commits.
delcypher edited subscribers, added: kcc, zaks.anna; removed: kubamracek.May 23 2016, 9:40 PM

Mike, please review. I won't have a chance tomorrow.

@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
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?

  1. 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 FIXMEs. 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.

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.

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?

  1. The CMakeLists files use flags that gcc doesn't understand. Is that intentional?
kcc added a comment.May 23 2016, 10:33 PM

@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

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?

  1. 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 FIXMEs. 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)?

  1. The CMakeLists files use flags that gcc doesn't understand. Is that intentional?

gcc does not have this flags, so yes, libFuzzer tests won't work with gcc.

In D20558#437510, @kcc 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?

  1. 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 FIXMEs. 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.

delcypher updated this revision to Diff 58282.May 24 2016, 11:54 AM
delcypher updated this object.
delcypher edited edge metadata.

Make sure that CMake disallows building without LLVM_USE_SANITIZER being set when
building LibFuzzer tests.

aizatsky added inline comments.May 24 2016, 4:18 PM
lib/Fuzzer/test/CMakeLists.txt
35

is -O always present? Do we want to always have "-O0" or do we want to rely on default level?

44

typo: SANITIZE

lib/Fuzzer/test/trace-bb/CMakeLists.txt
2โ€“3

let's remove it then.

lib/Fuzzer/test/trace-pc/CMakeLists.txt
2

Same. Let's remove it.

lib/Fuzzer/test/ubsan/CMakeLists.txt
3โ€“7

I guess it is HandleLLVMOptions in this case.

delcypher marked an inline comment as done.May 24 2016, 4:50 PM
delcypher added inline comments.May 24 2016, 4:55 PM
lib/Fuzzer/test/CMakeLists.txt
35

@aizatsky. It should be provided

  • The compiler is gcc/clang - We have a hard requirement on Clang anyway so this is fine.
  • LLVM hasn't previously modified the CMAKE_CXX_FLAGS_* from their default. AFAICT this holds too as LLVM usually modifies CMAKE_CXX_FLAGS rather than the release specific variables.

If you like I could put in a check for this assumption so we throw an error if this assumption doesn't hold.

delcypher updated this revision to Diff 58359.May 24 2016, 5:07 PM
  • Drop some incorrect comments
  • Convert some FIXMEs to comments stating where other -fsanitize-coverage= flags come from
delcypher marked 3 inline comments as done.May 24 2016, 5:56 PM

@aizatsky @kcc
Hopefully this is good to go now. I would like to make more changes to how the flags are handled but I think that should be done as a follow up commit because I don't want to change too much in one go.

delcypher updated this object.May 24 2016, 6:09 PM
delcypher marked 2 inline comments as done.May 24 2016, 10:30 PM
delcypher added inline comments.
lib/Fuzzer/test/CMakeLists.txt
35

Marking as done because I don't think the check is necessary. @aizatsky can ping if me if it is really necessary.

delcypher updated this revision to Diff 58386.May 24 2016, 10:52 PM
delcypher marked an inline comment as done.

Remove stray tab characters.

aizatsky edited edge metadata.May 25 2016, 1:04 PM

The build files are a little bit verbose now, but I guess that's the downside of having everything explicit.

The only serious sore to my eye is "-O" stripping but it is probably unfortunate.

LG as long as this works, but I'll let Kostya to sign off on this.

BTW I have added instructions to run libfuzzer tests here:

http://llvm.org/docs/LibFuzzer.html#developing-libfuzzer

kcc added a comment.May 25 2016, 1:12 PM

This is rather confusing. I stopped understanding why you are doing this and it looks like you are trying to solve several problems at once.

try to

  • split this patch into several, each patch solving just one problem (if possible)
  • make the cmake files smaller, not bigger
  • avoid non-actionable FIXMEs
lib/Fuzzer/CMakeLists.txt
10

Why is this FIXME? Do you expect to implement these flags in gcc?

13

Do you really need this FIXME? (up to you, just asking)

lib/Fuzzer/test/CMakeLists.txt
34

Do you need this FIXME>

109

This sucks. It really makes the file much bigger. Is that the only way?

delcypher added inline comments.May 25 2016, 1:45 PM
lib/Fuzzer/CMakeLists.txt
10

No the intention is to communicate to the reader that these flags aren't understood by gcc. It is FIXME because in the future we might allow building libFuzzer with gcc but disable building this tests (which can't be built with gcc).

13

It is something we should be testing for really. It just isn't a priority for me right now so it's a FIXME

lib/Fuzzer/test/CMakeLists.txt
34

Yes. This is communicating to the reader why we are doing "XX${flag}" instead of "${flag}. This is something that ought to be fixed but everyone has to be using a recent version of CMake and have a particular policy set in order to that which we can't do right now.

109

Whether or not "this" sucks is a matter of perspective. I don't think it does because it is adding a considerable amount of clarity. It is adding a few lines but I consider this a massive improvement to the way things were being done before.

Also I need this separation of compile and link flags because in some new patches that I'd like to upstream I need to pass some other flags to the linker to get things to build under OSX.

In D20558#439779, @kcc wrote:

This is rather confusing. I stopped understanding why you are doing this and it looks like you are trying to solve several problems at once.

try to

  • split this patch into several, each patch solving just one problem (if possible)

Although I can split a few things out doing so is going to be a pain because that means I am going has separate reviews going that are dependent on each other and I'm not sure if that really works in fabricator.

  • make the cmake files smaller, not bigger

That should not be a hard rule. If a change makes something clearer by adding a few lines I consider that to be the right trade-off

  • avoid non-actionable FIXMEs

All the FIXMEs are actionable.

I'm not sure where you want to go from here. I can split this patch up into to three bits but I'm not convinced it will possible to review them in phabricator because the changes are dependent on each other.

delcypher updated this revision to Diff 58559.May 25 2016, 9:20 PM
delcypher retitled this revision from [LibFuzzer] Start cleaning up the CMakeLists.txt files. to [LibFuzzer] Allow LibFuzzer to be built in modes other than RELEASE. .
delcypher updated this object.
delcypher edited edge metadata.

Completely rework patch with goal of keeping change minimal (despite the bad style)

@kcc @aizatsky : I've completely reworked to the patch to do just one thing. I'm not very happy with the current implementation as modifying CMAKE_CXX_FLAGS directly is not how modern CMake code should be written but to keep the change minimal this patch is following that style.

@aizatsky

BTW I have added instructions to run libfuzzer tests here:

http://llvm.org/docs/LibFuzzer.html#developing-libfuzzer

Thanks for doing this. If this patch gets merged requiring -DCMAKE_BUILD_TYPE=RELEASE will no longer be required.

kcc accepted this revision.May 26 2016, 9:55 AM
kcc edited edge metadata.

LGTM, assuming everything keeps working on Linux and Mac.

This revision is now accepted and ready to land.May 26 2016, 9:55 AM

@kcc : Thanks. It seems to work fine on Linux but the build currently fails under OSX but it was broken before this patch. I'll be upstreaming fixes for OSX soon

This revision was automatically updated to reflect the committed changes.