Page MenuHomePhabricator

llvm-shlib: Create object libraries for each component and link against them
Needs ReviewPublic

Authored by tstellar on Jan 29 2021, 9:36 PM.

Details

Summary

This makes it possible to build libLLVM.so without first creating a
static library for each component. In the case where only libLLVM.so is
built (i.e. ninja LLVM) this eliminates 150 linker jobs.

Diff Detail

Event Timeline

tstellar created this revision.Jan 29 2021, 9:36 PM
tstellar requested review of this revision.Jan 29 2021, 9:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2021, 9:36 PM
stellaraccident accepted this revision.Feb 6 2021, 9:20 PM

Nice - I had been considering this kind of change based on similar observations (prior to my larger refactor escapades).

This revision is now accepted and ready to land.Feb 6 2021, 9:20 PM
This revision was landed with ongoing or failed builds.Apr 1 2021, 3:02 PM
This revision was automatically updated to reflect the committed changes.
bjope added a subscriber: bjope.Apr 2 2021, 6:45 AM

Unfortunately out downstream bots started to fail with this commit (building the main branch).

Not sure exactly what happens, but my cmake command looks like this

cmake command: CC='/app/llvm/8.0/bin/clang -march=corei7' CXX='/app/llvm/8.0/bin/clang++ -march=corei7' LDFLAGS='-L/app/llvm/8.0/lib64 -Wl,-R/app/llvm/8.0/lib64:/z3/4.8.8-1/lib64' PATH=/app/ninja/1.8.2/bin:$PATH  /app/vbuild/RHEL7-x86_64/cmake/3.16.4/bin/cmake /repo/llvm-upstream/llvm --debug-trycompile -G Ninja -DCMAKE_MAKE_PROGRAM=/app/ninja/1.8.2/bin/ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_INSTALL_PREFIX=/compiler-clang -DLLVM_APPEND_VC_REV=OFF -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_WERROR=ON -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;lld' -DLLVM_ENABLE_Z3_SOLVER=ON -DLLVM_Z3_INSTALL_DIR=/z3/4.8.8-1 -DLLVM_ENABLE_LIBPFM=OFF

And with this patch I get:

FAILED: lib/Support/CMakeFiles/obj.LLVMSupport.dir/Z3Solver.cpp.o 
/app/llvm/8.0/bin/clang++  -march=corei7  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Support -I../lib/Support -Iinclude -I../include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3    -UNDEBUG -std=c++14  -fno-exceptions -fno-rtti -MD -MT lib/Support/CMakeFiles/obj.LLVMSupport.dir/Z3Solver.cpp.o -MF lib/Support/CMakeFiles/obj.LLVMSupport.dir/Z3Solver.cpp.o.d -o lib/Support/CMakeFiles/obj.LLVMSupport.dir/Z3Solver.cpp.o -c ../lib/Support/Z3Solver.cpp
../lib/Support/Z3Solver.cpp:19:10: fatal error: 'z3.h' file not found
#include <z3.h>
         ^~~~~~
1 error generated.

Without this patch I also see -isystem /z3/4.8.8-1/include on the cmd line for compiling Z3Solver.cpp.

bjope added a subscriber: uabelho.Apr 2 2021, 6:47 AM
bjope added a comment.Apr 3 2021, 1:53 AM

So this seem to break more things than just Z3 builds (considering D99829). Should we revert while sorting out these things. I'm kind of lost here, just know that our integration bots are stuck and they've been failing since this patch landed on April 1.

bjope added a comment.Apr 3 2021, 3:07 AM

Not sure if it is the correct thing to do, but a patch like this seem to help for the Z3 problem:

diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt
index ddacb4feaa0f..36919b25b0ef 100644
--- a/llvm/lib/Support/CMakeLists.txt
+++ b/llvm/lib/Support/CMakeLists.txt
@@ -274,8 +274,5 @@ if(LLVM_INTEGRATED_CRT_ALLOC)
 endif()
 
 if(LLVM_WITH_Z3)
-  target_include_directories(LLVMSupport SYSTEM
-    PRIVATE
-    ${Z3_INCLUDE_DIR}
-    )
+  include_directories(SYSTEM ${Z3_INCLUDE_DIR})
 endif()
bjope added a comment.Apr 3 2021, 3:29 AM

I pushed my workaround for Z3 here: https://reviews.llvm.org/rGd66f9c4f1e83e69abf75f97cb5f8fd1dc9422357
Hopefully that will make our bots happy again.

I don't mind if someone wants to do a post-commit review of my workaround.

I pushed my workaround for Z3 here: https://reviews.llvm.org/rGd66f9c4f1e83e69abf75f97cb5f8fd1dc9422357
Hopefully that will make our bots happy again.

I don't mind if someone wants to do a post-commit review of my workaround.

Thanks, let me know if you are still seeing issues.

tstellar reopened this revision.Apr 6 2021, 10:46 PM
This revision is now accepted and ready to land.Apr 6 2021, 10:46 PM
tstellar updated this revision to Diff 335725.Apr 6 2021, 10:48 PM

Updated patch with fixes for z3 and libffi build failures

https://bugs.llvm.org/show_bug.cgi?id=49818

In the case where only libLLVM.so is built (i.e. ninja LLVM) this eliminates 150 linker jobs.

Creating an archive does not need linker.

When Polly is in LLVM_ENABLE_PROJECTS, I get this

ld.lld: error: undefined symbol: getPollyPluginInfo()
>>> referenced by Extensions.cpp
>>>               lib/Extensions/CMakeFiles/obj.LLVMExtensions.dir/Extensions.cpp.o:(llvm::details::extensions_anchor())
>>> referenced by LTOBackend.cpp
>>>               lib/LTO/CMakeFiles/obj.LLVMLTO.dir/LTOBackend.cpp.o:(llvm::lto::opt(llvm::lto::Config const&, llvm::TargetMachine*, unsigned int, llvm::Module&, bool, llvm::ModuleSummaryIndex*, llvm::ModuleSummaryIndex const*, std::vector<unsigned char, std::allocator<unsigned char> > const&))

I find that in the linker command line all the .o files are linked without --start-lib. This means there will be no archive selection. Can this increase the output size?

phosek added a subscriber: phosek.Jun 15 2021, 12:10 AM

@tstellar what are the blockers for getting this landed?

When Polly is in LLVM_ENABLE_PROJECTS, I get this

ld.lld: error: undefined symbol: getPollyPluginInfo()
>>> referenced by Extensions.cpp
>>>               lib/Extensions/CMakeFiles/obj.LLVMExtensions.dir/Extensions.cpp.o:(llvm::details::extensions_anchor())
>>> referenced by LTOBackend.cpp
>>>               lib/LTO/CMakeFiles/obj.LLVMLTO.dir/LTOBackend.cpp.o:(llvm::lto::opt(llvm::lto::Config const&, llvm::TargetMachine*, unsigned int, llvm::Module&, bool, llvm::ModuleSummaryIndex*, llvm::ModuleSummaryIndex const*, std::vector<unsigned char, std::allocator<unsigned char> > const&))

I find that in the linker command line all the .o files are linked without --start-lib. This means there will be no archive selection. Can this increase the output size?

It does, unless you use -Wl,--gc-sections, that's one of the downsides of using object libraries over archives.

@phosek The only thing blocking this was that I wasn't sure what to do about @MaskRay 's comment. Other than that, I think all the regressions from before have been fixed.

beanz added a comment.Jun 15 2021, 8:39 AM

@phosek The only thing blocking this was that I wasn't sure what to do about @MaskRay 's comment. Other than that, I think all the regressions from before have been fixed.

The fix for that is probably to exclude libLLVMExtensions from the dylib. It probably shouldn't be there anyways.

In the case where only libLLVM.so is built (i.e. ninja LLVM) this eliminates 150 linker jobs.

Do you mean ar/llvm-ar?

It does, unless you use -Wl,--gc-sections, that's one of the downsides of using object libraries over archives.

Most non-SHF_ALLOC sections cannot be GCed.

If we want to save the copy/paste work done by ar/llvm-ar, can we use thin archives?

The fix for that is probably to exclude libLLVMExtensions from the dylib. It probably shouldn't be there anyways.

I think it is needed since the dylib references symbols in libLLVMExtensions for statically linked plugins, LLVMLTO depends on it. It was added to resolve build errors. @serge-sans-paille may know more.

tstellar updated this revision to Diff 381032.Oct 20 2021, 11:11 AM

Fix build failure with polly enabled.

tstellar requested review of this revision.Oct 20 2021, 11:12 AM

When Polly is in LLVM_ENABLE_PROJECTS, I get this

ld.lld: error: undefined symbol: getPollyPluginInfo()
>>> referenced by Extensions.cpp
>>>               lib/Extensions/CMakeFiles/obj.LLVMExtensions.dir/Extensions.cpp.o:(llvm::details::extensions_anchor())
>>> referenced by LTOBackend.cpp
>>>               lib/LTO/CMakeFiles/obj.LLVMLTO.dir/LTOBackend.cpp.o:(llvm::lto::opt(llvm::lto::Config const&, llvm::TargetMachine*, unsigned int, llvm::Module&, bool, llvm::ModuleSummaryIndex*, llvm::ModuleSummaryIndex const*, std::vector<unsigned char, std::allocator<unsigned char> > const&))

I find that in the linker command line all the .o files are linked without --start-lib. This means there will be no archive selection. Can this increase the output size?

Is --start-lib something specific to lld? I did builds with binutils and libLLVM.so was the same size with and without this patch.

MaskRay added a comment.EditedOct 20 2021, 2:18 PM

Is --start-lib something specific to lld? I did builds with binutils and libLLVM.so was the same size with and without this patch.

gold and ld.lld support --start-lib for a long time. (Bazel uses --start-lib by default.)
GNU ld feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=24600

Switching away from archive semantics (http://maskray.me/blog/2021-06-20-symbol-processing) has the problem that unused .debug_* may be added to the output with gold and ld.lld.

GNU ld may have a rarely known heuristic to not add .debug_* if all text section are garbage collected but I am not clear about its semantics.

I test this with LLVM_LINK_LLVM_DYLIB, and with Windows. both seem to work, so LGTM.

Is --start-lib something specific to lld? I did builds with binutils and libLLVM.so was the same size with and without this patch.

gold and ld.lld support --start-lib for a long time. (Bazel uses --start-lib by default.)
GNU ld feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=24600

Switching away from archive semantics (http://maskray.me/blog/2021-06-20-symbol-processing) has the problem that unused .debug_* may be added to the output with gold and ld.lld.

GNU ld may have a rarely known heuristic to not add .debug_* if all text section are garbage collected but I am not clear about its semantics.

What do I need to do in order to avoid changing the output of lld and gold with this patch? Can I wrap the whole list of object files with -Wl,--start-lib and -Wl,--end-lib or do I need to have multiple --start-lib and --end-lib pairs, one for each original static archive?

Is --start-lib something specific to lld? I did builds with binutils and libLLVM.so was the same size with and without this patch.

gold and ld.lld support --start-lib for a long time. (Bazel uses --start-lib by default.)
GNU ld feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=24600

Switching away from archive semantics (http://maskray.me/blog/2021-06-20-symbol-processing) has the problem that unused .debug_* may be added to the output with gold and ld.lld.

GNU ld may have a rarely known heuristic to not add .debug_* if all text section are garbage collected but I am not clear about its semantics.

What do I need to do in order to avoid changing the output of lld and gold with this patch? Can I wrap the whole list of object files with -Wl,--start-lib and -Wl,--end-lib or do I need to have multiple --start-lib and --end-lib pairs, one for each original static archive?

One pair of -Wl,--start-lib and -Wl,--end-lib for each original archive.

If you have -Wl,--start-lib a0.o a1.o -Wl,--end-lib -Wl,--start-lib b0.o b1.o -Wl,--end-lib, -Wl,--end-lib -Wl,--start-lib can be cancelled.

smeenai resigned from this revision.Apr 28 2022, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 2:00 PM

@tstellar Do you still plan to work on this change? If not, would it be fine with you if we commandeered the change?

@phosek You can take over, that's fine with me.