New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUILD] Fix LLVM libraries cmake symbol #1461

Merged
merged 1 commit into from Jul 23, 2018

Conversation

Projects
None yet
2 participants
@grwlf
Copy link
Contributor

grwlf commented Jul 20, 2018

Original issue discussion:
https://discuss.tvm.ai/t/llvm-error-option-registered-more-than-once-while-loading-libtvm-so/269/3

Reference:
CastXML/CastXML#102 (comment)

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from others in the community.

@grwlf

This comment has been minimized.

Copy link
Contributor Author

grwlf commented Jul 20, 2018

OK Probably one have to condition on LLVM versions here. I'll try to come with a solution. On my machine cmake is not able to build correct libraries without this.

@grwlf grwlf force-pushed the grwlf:fix-cmake-llvm branch from 7dd9a1c to 6bdbfa6 Jul 20, 2018

@tqchen

This comment has been minimized.

Copy link
Member

tqchen commented Jul 20, 2018

Unfortunately, we cannot simply use LLVM, can you investigate further by looking into what is in LLVM_LIBS.

The current rule makes the assumption that llvm is packed in libLLVM, which is not always true

@grwlf grwlf force-pushed the grwlf:fix-cmake-llvm branch 8 times, most recently from 5328af1 to 6911570 Jul 20, 2018

@grwlf

This comment has been minimized.

Copy link
Contributor Author

grwlf commented Jul 21, 2018

OK, I understand the concerns. On our machine LLVM_LIBS contains a long list of static library entries followed by a dynamic library entry, like this:

LLVMDemangle;LLVMSupport;LLVMTableGen;LLVMCore;...;LLVMXRay;LLVM

The initial error was caused by a singleton, which was executed twice: first time by a static part of llvm, coming from libtvm.so, second time by dynamic library. Note, that systems which don't distribute dynamic version of LLVM are not affected. Jenkins seems to be lucky in this sence.

For the affected systems, the problem may be solved by either 1) linking with only static llvm or 2) linking with only dynamic llvm. Initially we suggested the first approach, the latest patch implements the second one which also works.

Number 2 is also less invasive, since it changes only one line of code. From the other hand, this way we wouldn't be able to use libtvm.so with libLLVM.so in applications, since both libraries would try to execute singleton and the second attempt will fail.

$ g++  -std=c++11 src/argmax/argmax0.cpp  -ltvm -lLLVM 
$ ./a.out
: CommandLine Error: Option 'disable-symbolication' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

(in our case, -lLLVM was added only to demonstrate the issue, it is not actually required)

@grwlf

This comment has been minimized.

Copy link
Contributor Author

grwlf commented Jul 21, 2018

Probably, the best thing would be to use dynamic lib if it is available and fallback to static mode otherwise.

@tqchen

This comment has been minimized.

Copy link
Member

tqchen commented Jul 21, 2018

is it possible to detect the LLVM_LIBS first to see if both static and dynamic libs co-exists? The current way might not work if there is only dynamic library(and it get removed)

@tqchen tqchen self-requested a review Jul 21, 2018

@grwlf grwlf force-pushed the grwlf:fix-cmake-llvm branch from 6911570 to c48749d Jul 21, 2018

@grwlf

This comment has been minimized.

Copy link
Contributor Author

grwlf commented Jul 21, 2018

The latest patch prefers dynamic LLVM if it is available and defaults to static linkage otherwise. According to 'Build LLVM with CMake' guide, dynamic library contains all the components, so the basic presence check should be enough.

Just discovered, that Jenkins uses explicitly-defined llvm-config, like set(USE_LLVM llvm-config-4.0). So its build script bypasses the affected "USE_LLVM ON" branch completely. On my machine this option correctly selects the single dynamic library.

[BUILD] Fix LLVM static/dynamic link issue
This patch prevents libtvm.so from both containing static parts of LLVM
and link with libLLVM.so by removing the latter from link list.

@grwlf grwlf force-pushed the grwlf:fix-cmake-llvm branch from c48749d to f8babe2 Jul 21, 2018

@tqchen

tqchen approved these changes Jul 23, 2018

@tqchen tqchen merged commit 71a8d0c into dmlc:master Jul 23, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@grwlf grwlf deleted the grwlf:fix-cmake-llvm branch Jul 31, 2018

tqchen added a commit to tqchen/tvm that referenced this pull request Aug 4, 2018

[BUILD] Fix LLVM static/dynamic link issue (dmlc#1461)
This patch prevents libtvm.so from both containing static parts of LLVM
and link with libLLVM.so by removing the latter from link list.

grwlf added a commit to grwlf/tvm that referenced this pull request Aug 8, 2018

[BUILD] Fix LLVM static/dynamic link issue (dmlc#1461)
This patch prevents libtvm.so from both containing static parts of LLVM
and link with libLLVM.so by removing the latter from link list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment