This is an archive of the discontinued LLVM Phabricator instance.

Add libFuzzer shared object build output
AbandonedPublic

Authored by IanPudney on Jul 30 2020, 7:13 AM.

Details

Summary

This change adds a CMake rule to produce shared object versions of libFuzzer (no-main). Unlike the static library versions, these shared libraries do not have a copy of libc++ statically linked in. Doing so is theoretically possible for x86-64 (via hacks); however, it is not possible for i386 because i386 does not suppot mixing position-independent and non-position-independent code in the same library.

Diff Detail

Event Timeline

IanPudney created this revision.Jul 30 2020, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2020, 7:13 AM
Herald added subscribers: Restricted Project, mgorny. · View Herald Transcript
IanPudney requested review of this revision.Jul 30 2020, 7:13 AM
kcc added a comment.Jul 30 2020, 9:06 AM

Do we need a version for 32-bit at all?
Not having a private version of libc++ is likely to cause subtle stability issues.

(Matt, please help me with the review here)

Sticking just with x86_64 is possible; I actually have the code for that here, but it's a bit ugly:
https://reviews.llvm.org/differential/diff/281467/

However, what stability issues could dynamic linking cause? The change to privately link libc++ was only added in 2019, so things must have worked well before then:
https://github.com/llvm/llvm-project/commit/66c60d9d714bed88765c1575fbf57e7582fd27a3

I'm fine with whichever version you prefer.

kcc added a comment.Jul 31 2020, 9:41 AM

Sticking just with x86_64 is possible; I actually have the code for that here, but it's a bit ugly:
https://reviews.llvm.org/differential/diff/281467/

If you don't need 32-bit .so, let's just have it in 64-bit version (and this fact documented)

However, what stability issues could dynamic linking cause? The change to privately link libc++ was only added in 2019, so things must have worked well before then:
https://github.com/llvm/llvm-project/commit/66c60d9d714bed88765c1575fbf57e7582fd27a3

Things didn't work well and we had a few workarounds, some were since reverted.
The problem with a non-private libc++ is that the common libc++ is instrumented for coverage.
Since libFuzzer itself uses libc++, the coverage instrumentation code is executed while inside libFuzzer,
screwing everything up.

I'm fine with whichever version you prefer.

IanPudney updated this revision to Diff 282670.Aug 3 2020, 10:15 AM

Updated to skip building i386 output, but to statically link libc++.

Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2020, 10:15 AM
This revision is now accepted and ready to land.Aug 3 2020, 5:35 PM

When I patch this in locally and run ninja check-fuzzer, I get a linking error for both i386 and x86_64:

FAILED: lib/clang/12.0.0/lib/linux/libclang_rt.fuzzer_no_main-i386.so                                                          
...
/usr/bin/ld: projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.i386.dir/FuzzerDriver.cpp.o: in function `fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned int))':
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:130: undefined reference to `pthread_create'           
/usr/bin/ld: /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:130: undefined reference to `pthread_create'
/usr/bin/ld: projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.i386.dir/FuzzerFork.cpp.o: in function `fuzzer::FuzzWithFork(fuzzer::Random&, fuzzer::FuzzingOptions const&, std::vector<std::__cxx11::basic_stri
ng<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::vector<std::__cxx11::basic_stri
ng<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, int)':
/usr/local/google/home/mascasa/code/llvm-project/compiler-rt/lib/fuzzer/FuzzerFork.cpp:(.text+0xd6d): undefined reference to `pthread_create'
clang: error: linker command failed with exit code 1 (use -v to see invocation)                                                
[655/3462] Linking CXX shared library lib/clang/12.0.0/lib/linux/libclang_rt.fuzzer_no_main-x86_64.so                          


FAILED: lib/clang/12.0.0/lib/linux/libclang_rt.fuzzer_no_main-x86_64.so                                                        
...                                                                           
/usr/bin/ld: projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerDriver.cpp.o: in function `fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long))':
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:130: undefined reference to `pthread_create'           
/usr/bin/ld: /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:130: undefined reference to `pthread_create'
/usr/bin/ld: /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:130: undefined reference to `pthread_create'
/usr/bin/ld: projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerFork.cpp.o: in function `fuzzer::FuzzWithFork(fuzzer::Random&, fuzzer::FuzzingOptions const&, std::vector<std::__cxx11::basic_st
ring<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::vector<std::__cxx11::basic_st
ring<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, int)':
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:130: undefined reference to `pthread_create'

Ian, can you please take a look?

So it looks like the bug in question was pre-existing, and this change just exposes it. Specifically, when building with Ninja instead of Makefiles, the code path for statically linking libc++ isn't actually hit. Rather, the fallthrough "normal" code path is hit. I've confirmed this by verifying that the assemblies produced before my change with Ninja still want regular libc++.

IanPudney updated this revision to Diff 283040.Aug 4 2020, 3:28 PM

Adjusted rule to skip -z,defs when building under Ninja.

morehouse added inline comments.Aug 4 2020, 3:52 PM
compiler-rt/lib/fuzzer/CMakeLists.txt
61

Nit: Can we rename this to LIBFUZZER_SHARED_LINK_FLAGS and move it down next to the case where it is used?

IanPudney updated this revision to Diff 283071.Aug 4 2020, 5:23 PM
IanPudney marked an inline comment as done.
morehouse accepted this revision.Aug 4 2020, 5:30 PM

I'll land this tomorrow.

IanPudney planned changes to this revision.Aug 4 2020, 5:59 PM

While this now builds with Ninja, there are some rtti-related issues with linking.

IanPudney updated this revision to Diff 283090.Aug 4 2020, 6:20 PM

Issue fixed.

This revision is now accepted and ready to land.Aug 4 2020, 6:20 PM
This revision was landed with ongoing or failed builds.Aug 5 2020, 9:03 AM
This revision was automatically updated to reflect the committed changes.
jfb added a subscriber: jfb.Aug 5 2020, 11:27 AM
jfb added inline comments.
compiler-rt/lib/fuzzer/CMakeLists.txt
195

This is broken on non-Linux and non-Fuchsia architectures which don't have libstdc++, for example Darwin. Can you please fix or revert?

morehouse added inline comments.Aug 5 2020, 12:00 PM
compiler-rt/lib/fuzzer/CMakeLists.txt
195

Would it be sufficient to add the following?

if (DARWIN)
  list(APPEND LIBFUZZER_SHARED_LINK_LIBS "-lc++")
else()
  list(APPEND LIBFUZZER_SHARED_LINK_LIBS "-lstdc++")
endif()

Or are there more platforms that are missing libstdc++?

lebedev.ri added inline comments.
compiler-rt/lib/fuzzer/CMakeLists.txt
195

Why is this particular line hardcoding some particular C++ STD implementation in the first place?
Ignoring distro defaults, one can always pass -stdlib=libc++ to clang.

morehouse added inline comments.Aug 5 2020, 12:28 PM
compiler-rt/lib/fuzzer/CMakeLists.txt
197

This line was also failing on Android, where there is no pthreads library.

IanPudney added inline comments.Aug 5 2020, 12:29 PM
compiler-rt/lib/fuzzer/CMakeLists.txt
195

Passing -stdlib=libc++ into the CFLAGS of the rule doesn't solve the problem. The .so links, but at runtime it complains about missing symbols like _ZTVN10cxxabiv120si_class_type_infoE.

195

Added this in https://reviews.llvm.org/D85339. However, I don't have a Darwin machine to test with.

IanPudney updated this revision to Diff 283354.Aug 5 2020, 12:43 PM

Updated to use -lc++ instead of lstdc++ on Darwin.

IanPudney reopened this revision.Aug 5 2020, 12:44 PM
This revision is now accepted and ready to land.Aug 5 2020, 12:44 PM
lebedev.ri added inline comments.Aug 5 2020, 1:13 PM
compiler-rt/lib/fuzzer/CMakeLists.txt
198

This is too fragile, basically duplicates defaults from clang/lib/Driver/ToolChains
Is there no way to make compiler do the right thing?

IanPudney added inline comments.Aug 5 2020, 1:38 PM
compiler-rt/lib/fuzzer/CMakeLists.txt
198

I don't suppose you have any suggestions? I don't even know why libstdc++ isn't being linked in automatically.

IanPudney abandoned this revision.Dec 28 2020, 5:11 PM