This is an archive of the discontinued LLVM Phabricator instance.

Moving libFuzzer to compiler-rt
ClosedPublic

Authored by george.karpenkov on Aug 18 2017, 6:08 PM.

Details

Summary

Any suggestions on how to make the patch more readable are welcome :)
Since for git-svn and all the git clones those are different repositories, I'm not sure whether it's possible to present the moves as version control moves.

This change gets libFuzzer into Clang's toolchain installation as a byproduct.

This patch is tightly coupled to https://reviews.llvm.org/D36909 in Clang.

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski added inline comments.Aug 18 2017, 6:19 PM
cmake/config-ix.cmake
583 ↗(On Diff #111781)

AND OS_NAME MATCHES "Darwin|Linux" ?

cmake/config-ix.cmake
583 ↗(On Diff #111781)

Good point, the build fails on some other systems!

On Darwin build libFuzzer only for osx for now

george.karpenkov edited the summary of this revision. (Show Details)Aug 18 2017, 6:53 PM
george.karpenkov added subscribers: bogner, delcypher.
krytarowski added inline comments.Aug 18 2017, 7:21 PM
cmake/config-ix.cmake
583 ↗(On Diff #111781)

Thanks, I've ported it locally to NetBSD... however tests are broken for me.. I will hold on with merging this code.

A side notice but wouldn't it be better to make libFuzzer a completely separate component (like openmp)? I've seen people take interest in it outside LLVM toolchain.

A side notice but wouldn't it be better to make libFuzzer a completely separate component (like openmp)? I've seen people take interest in it outside LLVM toolchain.

libfuzzer coexists with sanitizers, it's a natural place for this feature in compiler-rt.

Ok, the first thing that I've noticed is that the compiler-rt version does not build with gcc:

c++: error: unrecognized command line option ‘-fno-sanitize-coverage=trace-pc-guard,edge,trace-cmp,indirect-calls,8bit-counters’

No clue how useful LLVMFuzzer built with gcc were but it certainly did build successfully.

I will move it here (from the mailing list):

This file test/fuzzer/lit.site.cfg.in contains redundant entries with the rest of infrastructure, notably config.cpp_compiler and config.c_compiler. This should be streamlined with the rest of the code

config.cpp_compiler = "@LIBFUZZER_TEST_COMPILER@"
config.target_flags = "@LIBFUZZER_TEST_FLAGS@"
config.c_compiler = "@LIBFUZZER_TEST_COMPILER@"

There are defines like COMPILER_RT_TEST_CXX_COMPILER and COMPILER_RT_TEST_COMPILER ready to use.

Please amend this patch with support for NetBSD:

http://www.netbsd.org/~kamil/llvm/libfuzzer-netbsd.txt

Not everything is ready, like missing lsan... but it's good enough for start and improve in future.

kcc edited edge metadata.Aug 19 2017, 6:42 PM

A side notice but wouldn't it be better to make libFuzzer a completely separate component (like openmp)? I've seen people take interest in it outside LLVM toolchain.

libFuzzer is already separate to a large extent.
You can build it outside of the LLVM tree (see build.sh) and use it w/o having LLVM (but you will need a compiler or some other tool to use libFuzzer-compatible coverage feedback).
But the libFuzzer development/testing is tightly integrated into the LLVM tool chain, and we don't expect this to change)

kcc added a comment.Aug 19 2017, 6:47 PM

Please amend this patch with support for NetBSD:

http://www.netbsd.org/~kamil/llvm/libfuzzer-netbsd.txt

Not everything is ready, like missing lsan... but it's good enough for start and improve in future.

Let's not overcomplicate the move to compiler-rt, it's pretty involved any way.
We would welcome support for any new platform in a separate patch once the dust settles (a week or two).
For any new supported platform will require a public build bot and a contact person.
Lacking that we can't promise to keep the port alive. (For further discussion, please start a new thread).

kcc added a comment.Aug 19 2017, 6:55 PM

This is the compiler-rt part, right?
Where is the llvm part that removes the code (did I miss it)?

For the LLVM part, please

  • delete all tests and cmake files
  • keep the libFuzzer sources and build.sh
  • add a README.txt explaining the temporary state (I'll remove the remnants once our internal users migrate, but will leave the README.txt for a while)

I am ok if you simply 'svn add' new files
Make sure the commit message contains instructions on how to obtain the svn history for the files prior to the move.

In D36908#846722, @kcc wrote:

Please amend this patch with support for NetBSD:

http://www.netbsd.org/~kamil/llvm/libfuzzer-netbsd.txt

Not everything is ready, like missing lsan... but it's good enough for start and improve in future.

Let's not overcomplicate the move to compiler-rt, it's pretty involved any way.
We would welcome support for any new platform in a separate patch once the dust settles (a week or two).
For any new supported platform will require a public build bot and a contact person.
Lacking that we can't promise to keep the port alive. (For further discussion, please start a new thread).

There is a bot and I'm the contact person:
http://lab.llvm.org:8011/builders/lldb-amd64-ninja-netbsd8

Attaching compiler-rt builds is now premature (sanitizers aren't upstreamed). My goal is to enable tests in LLVM, Clang and LLDB first.

To make it happen earlier (to get a compiler-rt buildbot) please help to review the sanitizers, for start: D35554 and D36587.

There is another thread mentioned by @mgorny that compiler-rt is broken for GCC builds (reported on bugzilla). I've filed a simple patch in D33878.

I think amending the lubfuzzer patch isn't burden here. That way I can also review and test it for the generic benefit (I don't use !NetBSD systems).

Thanks for collaboration.

kcc added a comment.Aug 19 2017, 7:03 PM

Any such change during a large refactoring is a burden, sorry.

In D36908#846732, @kcc wrote:

Any such change during a large refactoring is a burden, sorry.

The author asked me to elaborate on the patch and support issue (on a mail), this encouraged me to submit it.

kcc added a comment.Aug 19 2017, 7:12 PM

Rule of thumb: never mix refactoring with functional changes (or porting to new platforms, etc) in a single change.

In D36908#846734, @kcc wrote:

Rule of thumb: never mix refactoring with functional changes (or porting to new platforms, etc) in a single change.

OK, thanks - I will wait for this code to be fully merged. I will provide a bot setup for compiler-rt/NetBSD to make the maintenance easier... just upstreaming and fixes take time - please be patient.

Ok, the first thing that I've noticed is that the compiler-rt version does not build with gcc:

@mgorny this is actually due to change proposed in https://reviews.llvm.org/D36887.
I am not sure what is the correct strategy here: previously, the flag was behind the guard, which was never set for gcc.
I don't think it is correct to rely on LLVM_* variable from a compiler-rt repository.
I can also test whether the flag is available first before compiling.

kcc added a comment.Aug 21 2017, 11:18 AM

Once we move to compiler-rt, the test will be build with the fresh clang.
But what about libFuzzer itself? Will it build with fresh clang or with the host compiler?
If 'fresh clang', then this is not a big deal for the LLVM build tree.
(We do need to keep libFuzzer buildable with fresh GCC though, but this problem is not about GCC in general, just about using GCC in the LLVM tree build).

@kcc compiler-rt folder builds both from projects and from runtimes.
In former case, it is build using the host compiler, and in latter, using freshly built Clang.
The latter case is arguably more "correct", yet that's not how all systems are configured.

Ok, the first thing that I've noticed is that the compiler-rt version does not build with gcc:

@mgorny this is actually due to change proposed in https://reviews.llvm.org/D36887.
I am not sure what is the correct strategy here: previously, the flag was behind the guard, which was never set for gcc.
I don't think it is correct to rely on LLVM_* variable from a compiler-rt repository.
I can also test whether the flag is available first before compiling.

Ah, I see. Thanks for the explanation. However, I don't think the limitation on LLVM_* variables really applies here. After all, compiler-rt is built using LLVM CMake files that use such variables on their own. In fact, compiler-rt already uses LLVM_USE_SANITIZER in config-ix.cmake, so I see no reason why it wouldn't use the latter.

Checking flag is also fine here. However, I'd personally prefer if you tried to move stuff as 1:1 as possible in this patch, and only did the changes strictly necessary to keep things working. This change is not strictly necessary, so it'd be better to split it into a followup patch.

vitalybuka edited edge metadata.Aug 21 2017, 11:27 AM

Could you please spit this patch in two:

  1. Move code from llvm to compile-rt as-is. No changes in cmake files and no hooking this into compiler-rt cmake files
  2. cmake and other changes necessary to build

@vitalybuka: OK. Note that nothing would work in the just-moving patch though (ideally I would be able to simply upload my branch)

Update the DIFF not to include moving the actual files.

@vitalybuka unfortunately, the compilation system required moving tests around (e.g. splitting into lib/fuzzer/test and test/fuzzer).

This diff requires https://reviews.llvm.org/D36974 for testing application.

@mgorny should now compile under GCC. The change in question is already moved out into https://reviews.llvm.org/D36887: the diff attached to this PR is what needs to happen after the move itself is performed.

kcc accepted this revision.Aug 21 2017, 1:43 PM

LGTM
Please update the commit message.

This revision is now accepted and ready to land.Aug 21 2017, 1:43 PM

@krytarowski

There are defines like COMPILER_RT_TEST_CXX_COMPILER and COMPILER_RT_TEST_COMPILER ready to use

Yes. In fact, they are already being used: LIBFUZZER_TEST_COMPILER is populated by get_test_cc_for_arch, which uses COMPILER_RT_TEST_COMPILER.

@kcc @vitalybuka just found a bug: standalone.test no longer works, because the path to libLLVMFuzzerNoMain.a becomes OS- and ARCH-dependent.
I can see two possible solutions:
either

a) Add e.g. -fsanitize=fuzzer-no-main to the driver (links in libclang_rt.fuzzer_no_main*)
or
b) Rewrite standalone.test as a unittest, compiled using CMake.

Doing (a) is easier, but there's a question of how much should we pollute the driver.
Should this be exposed to users? Is it useful to be able to link in a library without overwriting main?

kcc added a comment.Aug 21 2017, 3:55 PM

@kcc @vitalybuka just found a bug: standalone.test no longer works, because the path to libLLVMFuzzerNoMain.a becomes OS- and ARCH-dependent.

This was introduced by https://reviews.llvm.org/D36295#db384381
In this test we should not link libFuzzer, so just remove %build_dir/lib/libLLVMFuzzerNoMain.a

kcc accepted this revision.Aug 21 2017, 4:15 PM

LGTM

This revision was automatically updated to reflect the committed changes.