This is an archive of the discontinued LLVM Phabricator instance.

Change llvm_add_library to always use name for static libraries
Needs ReviewPublic

Authored by winksaville on May 16 2019, 2:17 PM.

Details

Reviewers
tstellar
beanz
Summary

Problem:

The default for libraries built using llvm_add_library is STATIC unless
BUILD_SHARED_LIBS is ON but having BUILD_SHARED_LIBS is an unsupported
configuration for non LLVM developers. See Note for BUILD_SHARED_LIBS:BOOL
in https://llvm.org/docs/CMake.html where it says:

"BUILD_SHARED_LIBS is only recommended for use by LLVM developers. If you
want to build LLVM as a shared library, you should use the
LLVM_BUILD_LLVM_DYLIB option."

As well as building STATIC libraries llvm_add_library can be used to
create SHARED, OBJECT or MODULE libraries. When creating these the first
parameter, ${name} is used as the name of the cmake object and also its
OUTPUT_NAME.

There is one exception to the naming rule, if both STATIC and SHARED are
passed to llvm_add_library both a static and shared libraries are created
and they must have different names. The current code uses ${name} for the
shared library and ${name}_static for the static library.

This causes a problem because by default LLVM sub-projects prefer linking
to LLVM using its static libraries, libLLVM*.a. For example, when clang
does this. But, if one of libclang* libraries is built by passing STATIC
and SHARED to llvm_add_library clang compiles and links just fine, but
when it is run we get an error:

$ ./bin/clang --version
: CommandLine Error: Option 'use-dbg-addr' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

This happens because there are two copies of libLLVM, the static version
linked to the statically linked libclang*.a libraries and the shared
version linked to the shared libclangXxx.so.

Solution:

Use ${name} for the STATIC library and ${name}_shared for the SHARED
library. With this change even if both STATIC and SHARED versions of a
particular library is created ${name} is the static version and there
is no error.

Event Timeline

winksaville created this revision.May 16 2019, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2019, 2:17 PM
beanz added a comment.May 16 2019, 2:58 PM

Why are the clang libraries being built STATIC and SHARED?

We need to understand the problem you are trying to solve, because this doesn't seem like the right solution.

Why are the clang libraries being built STATIC and SHARED?

We need to understand the problem you are trying to solve, because this doesn't seem like the right solution.

Building STATIC and SHARED libraries is a capability already available in llvm_add_library (see recursive call at line 451)
and this patch fixes the bug I found while using that capability to create D61804 "Support building shared and static libraries simultaneously"
for clang. As it turns out your solution was better, D61909, so D61804 was abandoned.

This patch is a subset of D61804 that only fixes the bug.

beanz added a comment.May 16 2019, 4:15 PM

I don't know that we want to support building component libraries as both STATIC and SHARED except in very specific and limited situations.

We actually don't seem to use this functionality in-tree anywhere, so I'm more inclined to turn this into an error than to fix it.

I don't know that we want to support building component libraries as both STATIC and SHARED except in very specific and limited situations.

We actually don't seem to use this functionality in-tree anywhere, so I'm more inclined to turn this into an error than to fix it.

I too had looked and found no place where it was used. But it does automatically use OBJECT so if and when it is used
".o's" aren't built twice. I suspect there are currently places in the build system which build both and don't use OBJECT
and thus are taking more time and space than necessary. So I'd vote to keep the capability, but if not then your suggestion
of turning it into an error is also a solution.

If turning this into an error is what you want to do I can prepare a patch, or you can do it, your choice.

beanz added a comment.May 16 2019, 4:58 PM

I don't know of anywhere that we use llvm_add_library to generate both static and shared libraries of the same code. The runtime libraries that are generated both ways roll their own solution to the problem.

I'm disinclined to see this fixed without an actual use case.

I disagree but I'm the neophyte :)

So do you want me to change it to an error or abandon this patch?

beanz added a comment.May 16 2019, 9:19 PM

LLVM generally has a policy of not leaving dead or unused code in tree. Changing to an error seems like the right approach given our coding guidelines.

OK, I'll do that tomorrow.