This is an archive of the discontinued LLVM Phabricator instance.

[Fuzzer] Avoid the unnecessary rebuild of the custom libc++
ClosedPublic

Authored by phosek on Feb 12 2018, 4:00 PM.

Details

Summary

This changes the add_custom_libcxx macro to resemble the
llvm_ExternalProject_Add. The primary motivation is to avoid
unnecessary libFuzzer rebuilds that are being done on every
Ninja/Make invocation. The libc++ should be only rebuilt whenever
the libc++ source itself changes.

Diff Detail

Event Timeline

phosek created this revision.Feb 12 2018, 4:00 PM
vitalybuka added inline comments.Feb 21 2018, 3:01 PM
compiler-rt/cmake/Modules/AddCompilerRT.cmake
530–531

Can you please add into patch description explanation when it is expected to be rebuilt

541

Have you considered to achieve the same with just:
BUILD_ALWAYS 0
BUILD_BYPRODUCTS <libs>

543

Why do you need USES_TERMINAL_ ?

compiler-rt/lib/fuzzer/CMakeLists.txt
89

Can this be just ${dir}/src/libcxx_fuzzer_${arch}/lib/libc++.a ?

compiler-rt/test/tsan/lit.cfg
60

why do we need "-bins" suffix?

Why not just call llvm_ExternalProject_Add rather than duplicating here?

compiler-rt/cmake/Modules/AddCompilerRT.cmake
556

Unused if.

Why not just call llvm_ExternalProject_Add rather than duplicating here?

Don't we still want to support the setup where compiler-rt is built as a standalone project in which case llvm_ExternalProject_Add wouldn't be available?

Why not just call llvm_ExternalProject_Add rather than duplicating here?

Don't we still want to support the setup where compiler-rt is built as a standalone project in which case llvm_ExternalProject_Add wouldn't be available?

In that case, llvm_ExternalProject_BuildCmd won't be available either but is still used by this patch.

Why not just call llvm_ExternalProject_Add rather than duplicating here?

Don't we still want to support the setup where compiler-rt is built as a standalone project in which case llvm_ExternalProject_Add wouldn't be available?

In that case, llvm_ExternalProject_BuildCmd won't be available either but is still used by this patch.

That's a mistake, thanks for catching that!

phosek updated this revision to Diff 136029.Feb 26 2018, 7:41 PM
phosek marked 3 inline comments as done.
phosek edited the summary of this revision. (Show Details)
phosek added inline comments.
compiler-rt/cmake/Modules/AddCompilerRT.cmake
541

The problem is that we would need to list all the library artifacts, e.g. libc++.so is only a symlink to libc++.so.1 which is a symlink to libc++.so.1.0, and I don't know if we want to rely on those details here?

543

This is not really needed, but now that libc++ is being used as part of the libFuzzer build, I just found it much easier to debug build issues if the libFuzzer's libc++ build output goes to terminal rather than file log which requires extra steps to read, I'm happy to drop this if you don't like it though.

compiler-rt/lib/fuzzer/CMakeLists.txt
89

That directory no longer exists, all the binary artifacts now end up in the -bins directory.

compiler-rt/test/tsan/lit.cfg
60

I was following the naming convention used by llvm_ExternalProject_Add, but I can drop the "-bins" suffix if you prefer?

morehouse added inline comments.Feb 27 2018, 9:32 AM
compiler-rt/cmake/Modules/AddCompilerRT.cmake
522

Why don't we depend on LIBCXX_DEPS here?

vitalybuka added inline comments.Feb 27 2018, 10:11 AM
compiler-rt/cmake/Modules/AddCompilerRT.cmake
543

up to you.

compiler-rt/lib/fuzzer/CMakeLists.txt
89

So why to use ${dir}-bins instead of ${dir}?
I'd prefer the name without suffix here

compiler-rt/test/tsan/lit.cfg
60

This is the primary location which will be used for linking? i'd prefer without suffix

phosek updated this revision to Diff 136236.Feb 27 2018, 11:55 PM
phosek marked 8 inline comments as done.
phosek updated this revision to Diff 136238.Feb 28 2018, 12:13 AM
phosek marked an inline comment as done.
vitalybuka accepted this revision.Mar 2 2018, 10:32 AM
This revision is now accepted and ready to land.Mar 2 2018, 10:32 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.