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

[delcypher]

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.

[kcc]

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

[delcypher]

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

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

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

[kcc]

In 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

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

3. 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.

[delcypher]

In D20558#437510, @kcc wrote:

In 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 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]

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

[aizatsky]

lib/Fuzzer/test/CMakeLists.txt

35 ↗

(On Diff #58282)

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

44 ↗

(On Diff #58282)

typo: SANITIZE

lib/Fuzzer/test/trace-bb/CMakeLists.txt

2 ↗

(On Diff #58282)

let's remove it then.

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

2 ↗

(On Diff #58282)

Same. Let's remove it.

lib/Fuzzer/test/ubsan/CMakeLists.txt

3 ↗

(On Diff #58282)

I guess it is HandleLLVMOptions in this case.

[delcypher]

Fix typo

[delcypher]

lib/Fuzzer/test/CMakeLists.txt

35 ↗

(On Diff #58355)

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

• Drop some incorrect comments
• Convert some FIXMEs to comments stating where other -fsanitize-coverage= flags come from

[delcypher]

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

lib/Fuzzer/test/CMakeLists.txt

35 ↗

(On Diff #58359)

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

[delcypher]

Remove stray tab characters.

[aizatsky]

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]

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 ↗

(On Diff #58386)

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

13 ↗

(On Diff #58386)

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

lib/Fuzzer/test/CMakeLists.txt

34 ↗

(On Diff #58386)

Do you need this FIXME>

110 ↗

(On Diff #58386)

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

[delcypher]

lib/Fuzzer/CMakeLists.txt

10 ↗

(On Diff #58386)

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 ↗

(On Diff #58386)

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 ↗

(On Diff #58386)

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.

110 ↗

(On Diff #58386)

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.

[delcypher]

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]

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

[delcypher]

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

[delcypher]

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

LGTM, assuming everything keeps working on Linux and Mac.

[delcypher]

@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

llvm/trunk/lib/Fuzzer/CMakeLists.txt

-set(LIBFUZZER_FLAGS_BASE "${CMAKE_CXX_FLAGS_RELEASE}")
+set(LIBFUZZER_FLAGS_BASE "${CMAKE_CXX_FLAGS}")
 # Disable the coverage and sanitizer instrumentation for the fuzzer itself.
-set(CMAKE_CXX_FLAGS_RELEASE "${LIBFUZZER_FLAGS_BASE} -O2 -fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters -Werror")
+set(CMAKE_CXX_FLAGS "${LIBFUZZER_FLAGS_BASE} -fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters -Werror")
 if( LLVM_USE_SANITIZE_COVERAGE )
   add_library(LLVMFuzzerNoMainObjects OBJECT
     FuzzerCrossOver.cpp
     FuzzerTraceState.cpp
     FuzzerDriver.cpp
     FuzzerIO.cpp
     FuzzerLoop.cpp
     FuzzerMutate.cpp

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

 # Build all these tests with -O0, otherwise optimizations may merge some
 # basic blocks and we'll fail to discover the targets.
-# Also enable the coverage instrumentation back (it is disabled
-# for the Fuzzer lib)
-set(CMAKE_CXX_FLAGS_RELEASE "${LIBFUZZER_FLAGS_BASE} -O0 -fsanitize-coverage=edge,indirect-calls")
+# We change the flags for every build type because we might be doing
+# a multi-configuration build (e.g. Xcode) where CMAKE_BUILD_TYPE doesn't
+# mean anything.
+set(variables_to_filter
+  CMAKE_CXX_FLAGS_RELEASE
+  CMAKE_CXX_FLAGS_DEBUG
+  CMAKE_CXX_FLAGS_RELWITHDEBINFO
+  CMAKE_CXX_FLAGS_MINSIZEREL
+  LIBFUZZER_FLAGS_BASE
+  )
+foreach (VARNAME ${variables_to_filter})
+  string(REPLACE " " ";" BUILD_FLAGS_AS_LIST "${${VARNAME}}")
+  set(new_flags "")
+  foreach (flag ${BUILD_FLAGS_AS_LIST})
+    # NOTE: Use of XX here is to avoid a CMake warning due to CMP0054
+    if (NOT ("XX${flag}" MATCHES "XX-O[0123s]"))
+      set(new_flags "${new_flags} ${flag}")
+    else()
+      set(new_flags "${new_flags} -O0")
+    endif()
+  endforeach()
+  set(${VARNAME} "${new_flags}")
+endforeach()
+
+# Enable the coverage instrumentation (it is disabled for the Fuzzer lib).
+set(CMAKE_CXX_FLAGS "${LIBFUZZER_FLAGS_BASE} -fsanitize-coverage=edge,indirect-calls")
 
 set(DFSanTests
   MemcmpTest
   SimpleCmpTest
   StrcmpTest
   StrncmpTest
   SwitchTest
   )

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

 # These tests depend on both coverage and dfsan instrumentation.
 
-set(CMAKE_CXX_FLAGS_RELEASE
-  "${LIBFUZZER_FLAGS_BASE} -O0 -fno-sanitize=all -fsanitize=dataflow")
+set(CMAKE_CXX_FLAGS
+  "${LIBFUZZER_FLAGS_BASE} -fno-sanitize=all -fsanitize=dataflow")
 
 foreach(Test ${DFSanTests})
   add_executable(LLVMFuzzer-${Test}-DFSan
     ../${Test}.cpp
     )
   target_link_libraries(LLVMFuzzer-${Test}-DFSan
     LLVMFuzzer
     )
 endforeach()
 

llvm/trunk/lib/Fuzzer/test/trace-bb/CMakeLists.txt

 # These tests are not instrumented with coverage.
 
-set(CMAKE_CXX_FLAGS_RELEASE
+set(CMAKE_CXX_FLAGS
   "${LIBFUZZER_FLAGS_BASE} -fsanitize-coverage=edge,trace-bb")
 
 foreach(Test ${TraceBBTests})
   add_executable(LLVMFuzzer-${Test}-TraceBB
     ../${Test}.cpp
     )
   target_link_libraries(LLVMFuzzer-${Test}-TraceBB
     LLVMFuzzer
     )
 endforeach()
 

llvm/trunk/lib/Fuzzer/test/trace-pc/CMakeLists.txt

 # These tests are not instrumented with coverage.
 
-set(CMAKE_CXX_FLAGS_RELEASE
-  "${LIBFUZZER_FLAGS_BASE} -O0 -fno-sanitize-coverage=8bit-counters -fsanitize-coverage=trace-pc")
+set(CMAKE_CXX_FLAGS
+  "${LIBFUZZER_FLAGS_BASE} -fno-sanitize-coverage=8bit-counters -fsanitize-coverage=trace-pc")
 
 foreach(Test ${TracePCTests})
   add_executable(LLVMFuzzer-${Test}-TracePC
     ../${Test}.cpp
     )
   target_link_libraries(LLVMFuzzer-${Test}-TracePC
     LLVMFuzzer
     )
 endforeach()
 

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

 # These tests are instrumented with ubsan in non-recovery mode.
 
-set(CMAKE_CXX_FLAGS_RELEASE
-  "${LIBFUZZER_FLAGS_BASE} -O0 -fsanitize=undefined -fno-sanitize-recover=all")
+set(CMAKE_CXX_FLAGS
+  "${LIBFUZZER_FLAGS_BASE} -fsanitize=undefined -fno-sanitize-recover=all")
 
 foreach(Test ${UbsanTests})
   add_executable(LLVMFuzzer-${Test}-Ubsan
     ../${Test}.cpp
     )
   target_link_libraries(LLVMFuzzer-${Test}-Ubsan
     LLVMFuzzer
     )
 endforeach()
 

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

 # These tests are not instrumented with coverage.
 
-set(CMAKE_CXX_FLAGS_RELEASE
-  "${LIBFUZZER_FLAGS_BASE} -O0 -fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters")
+set(CMAKE_CXX_FLAGS
+  "${LIBFUZZER_FLAGS_BASE} -fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters")
 
 foreach(Test ${UninstrumentedTests})
   add_executable(LLVMFuzzer-${Test}-Uninstrumented
     ../${Test}.cpp
     )
   target_link_libraries(LLVMFuzzer-${Test}-Uninstrumented
     LLVMFuzzer
     )
 endforeach()