[PATCH] D12488: Enable linking tools, shared libraries against libLLVM

Thu Sep 3 09:49:01 PDT 2015

> On Sep 2, 2015, at 6:11 PM, Andrew Wilkins <axwalk at gmail.com> wrote:
> 
> axw added a comment.
> 
> In https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D12488-23238489&d=BQIFaQ&c=eEvniauFctOgLOKGJOplqw&r=Hfcbhu5JC70tzQtVGDgDJg&m=aFzMU9u5okwXLNXj4X9H4NKFs0MiX_i8EJYtIhE20MM&s=w0tSfSDe8NEYlcs9ef8bFWCuQn8L-n4DaT1b7aVswTg&e= , @beanz wrote:
> 
>> Clang builds, but it doesn’t link libLLVM. See:
>> 
>>> grep "build bin/clang-3.8" ./build.ninja
>>   build bin/clang-3.8: CXX_EXECUTABLE_LINKER__clang tools/clang/tools/driver/CMakeFiles/clang.dir/driver.cpp.o tools/clang/tools/driver/CMakeFiles/clang.dir/cc1_main.cpp.o tools/clang/tools/driver/CMakeFiles/clang.dir/cc1as_main.cpp.o | lib/libLLVM.3.8.0svn.dylib lib/libclangBasic.a lib/libclangCodeGen.a lib/libclangDriver.a lib/libclangFrontend.a lib/libclangFrontendTool.a lib/libclangCodeGen.a lib/libLLVMBitWriter.a lib/libLLVMipo.a lib/libLLVMVectorize.a lib/libLLVMIRReader.a lib/libLLVMAsmParser.a lib/libLLVMInstrumentation.a lib/libLLVMLinker.a lib/libLLVMObjCARCOpts.a lib/libLLVMProfileData.a lib/libLLVMObject.a lib/libLLVMScalarOpts.a lib/libLLVMInstCombine.a lib/libLLVMTarget.a lib/libLLVMTransformUtils.a lib/libLLVMAnalysis.a lib/libclangRewriteFrontend.a lib/libclangARCMigrate.a lib/libclangStaticAnalyzerFrontend.a lib/libclangFrontend.a lib/libclangDriver.a lib/libLLVMOption.a lib/libclangParse.a lib/libLLVMMCParser.a lib/libclangSerialization.a lib/libLLVMBitReader.a lib/libclangSema.a lib/libclangEdit.a lib/libclangStaticAnalyzerCheckers.a lib/libclangStaticAnalyzerCore.a lib/libclangAnalysis.a lib/libclangAST.a lib/libclangRewrite.a lib/libclangLex.a lib/libclangBasic.a lib/libLLVMCore.a lib/libLLVMMC.a lib/libLLVMSupport.a || lib/libLLVMInstCombine.a lib/libLLVMAsmParser.a lib/libLLVMProfileData.a lib/libLLVMScalarOpts.a lib/libLLVMipo.a lib/libLLVMVectorize.a lib/libLLVMObjCARCOpts.a lib/libLLVMLinker.a lib/libLLVMAnalysis.a lib/libLLVMMC.a lib/libLLVMMCParser.a lib/libLLVMObject.a lib/libLLVMOption.a lib/libLLVMTarget.a tools/clang/include/clang/AST/ClangCommentCommandList tools/clang/include/clang/AST/ClangCommentCommandInfo tools/clang/include/clang/AST/ClangCommentHTMLNamedCharacterReferences tools/clang/include/clang/AST/ClangCommentHTMLTagsProperties tools/clang/include/clang/AST/ClangCommentNodes tools/clang/include/clang/AST/ClangAttrImpl tools/clang/include/clang/AST/ClangStmtNodes tools/clang/include/clang/AST/ClangAttrClasses tools/clang/include/clang/AST/ClangAttrDump tools/clang/include/clang/AST/ClangCommentHTMLTags tools/clang/include/clang/AST/ClangDeclNodes tools/clang/include/clang/AST/ClangAttrVisitor tools/clang/include/clang/Basic/ClangARMNeon tools/clang/include/clang/Basic/ClangDiagnosticIndexName tools/clang/include/clang/Basic/ClangDiagnosticParse tools/clang/include/clang/Basic/ClangDiagnosticFrontend tools/clang/include/clang/Basic/ClangDiagnosticSerialization tools/clang/include/clang/Basic/ClangDiagnosticLex tools/clang/include/clang/Basic/ClangDiagnosticCommon tools/clang/include/clang/Basic/ClangDiagnosticSema tools/clang/include/clang/Basic/ClangAttrList tools/clang/include/clang/Basic/ClangDiagnosticComment tools/clang/include/clang/Basic/ClangAttrHasAttributeImpl tools/clang/include/clang/Basic/ClangDiagnosticAST tools/clang/include/clang/Basic/ClangDiagnosticDriver tools/clang/include/clang/Basic/ClangDiagnosticGroups tools/clang/include/clang/Basic/ClangDiagnosticAnalysis tools/clang/include/clang/Parse/ClangAttrParserStringSwitches tools/clang/include/clang/Sema/ClangAttrParsedAttrList tools/clang/include/clang/Sema/ClangAttrTemplateInstantiate tools/clang/include/clang/Sema/ClangAttrSpellingListIndex tools/clang/include/clang/Sema/ClangAttrParsedAttrImpl tools/clang/include/clang/Sema/ClangAttrParsedAttrKinds tools/clang/include/clang/Serialization/ClangAttrPCHWrite tools/clang/include/clang/Serialization/ClangAttrPCHRead tools/clang/lib/Headers/clang-headers lib/libclangBasic.a lib/libclangLex.a lib/libclangParse.a lib/libclangAST.a lib/libclangSema.a lib/libclangCodeGen.a lib/libclangAnalysis.a lib/libclangEdit.a lib/libclangRewrite.a lib/libclangARCMigrate.a lib/libclangDriver.a lib/libclangSerialization.a lib/libclangFrontend.a lib/libclangRewriteFrontend.a lib/libclangFrontendTool.a lib/libclangStaticAnalyzerCore.a lib/libclangStaticAnalyzerCheckers.a lib/libclangStaticAnalyzerFrontend.a lib/libLLVM.dylib lib/libLLVMSupport.a lib/libLLVMCore.a lib/libLLVMIRReader.a lib/libLLVMBitReader.a lib/libLLVMBitWriter.a lib/libLLVMTransformUtils.a lib/libLLVMInstrumentation.a
>> 
> 
> 
> libLLVM.dylib *is* in that list (after clang static archives, before LLVM static archives). What am I missing?
> (Sorry, I'd try on OS X myself but I don't have it. I also don't really know it well, so my bafflement could be related.)

You’re right. My earlier grep regex was broken which led me to believe it wasn’t in there. My bad.

> 
>> It is actually impossible for your change to work as implemented because the libLLVM targets don’t exist until after clang, lld, and lldb targets are generated.
> 
> 
> Again, this *is* working for me on Linux. Not theoretical - I'm looking at the resulting binaries right now, and all them are linked against libLLVM.so.
> 
> I am under the impression that CMake builds up fake nodes for libraries referred to ahead of time, and replaces them in the DAG when
> the target is defined for real, so that ordering wouldn't matter. I don't know how this would work for me at all otherwise. I just wrote a
> small CMake test that does:
> 
> - add_executable(X ...)
> - target_link_libraries(X Y)
> - add_library(Y ...)
> 
> and it worked just fine.

I would have sworn we did some odd stuff in add_llvm_* that relied on targets already being created, but I don’t see anything in there, so I’m apparently losing my marbles.

> 
>> See the comment at the top of tools/CMakeLists.txt WRT ordering issues.
> 
>> 
> 
>> One of the other problems that you should account for is that not all tools and subprojects use AddLLVM. The AddLLVM macros shouldn't be calling `target_link_libraries` for LLVM libraries (I know the code did that in some places already and I'm not asking you to clean it up, just don't make it worse). Instead we should be relying on `llvm_config`. We should add an option "USE_SHARED" to `llvm_config`, and we should have code that filters the list of components passed into `explicit_llvm_config` based on the components included in the dylib and calls `target_link_libraries` as appropriate.
> 
> 
> Sounds sensible. I don't quite understand why you'd need to filter the components, since they'll just be ignored if USE_SHARED is set?

Pretty much every linker in the world implements symbol lookup in the order specified, so it isn’t strictly required to function, but years of debugging build system issues has taught me that throwing extra things on your command line just muddles things and makes it harder to see what is going wrong when things break.

> 
>> For this to work there are two changes required further out in CMake. (1) LLVM_DYLIB_COMPONENTS needs to be defined in the root CMakeLists (it can default to 'all'), and (2) tools/CMakeLists.txt needs to explicitly add llvm-shlib before any other tools.
> 
>> 
> 
>> It is probably reasonable to have `add_llvm_utility` default to not using the shared library, as I can't think of a single utility that is shipped as part of a distribution (and if there is one we should probably make it a tool).
> 
> 
> Thanks for explaining, that sounds neater if nothing else. I'll put something together soon that addresses these points, and send it to you for review. Sorry for causing you grief.

Thank you, I saw your patch in my inbox, I’ll try to get to it this morning.

-Chris

> 
> 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D12488&d=BQIFaQ&c=eEvniauFctOgLOKGJOplqw&r=Hfcbhu5JC70tzQtVGDgDJg&m=aFzMU9u5okwXLNXj4X9H4NKFs0MiX_i8EJYtIhE20MM&s=etl6Qn-5SaklFxOYJyYLZFPzDgE911DqP4cKcgmU89Y&e= 
> 
> 
> 