Uh oh!
There was an error while loading. Please reload this page.
[CMake] Add compile-time check that .so files have no undefined symbols - #8178
Conversation
Lunderberg
commented
Jun 2, 2021
Potential reviewers: @areusch as this impacts the CI. |
areusch
left a comment
There was a problem hiding this comment.
thanks for adding this, @Lunderberg . i'm supportive of this, would be great to get opinions from those who work more with external libraries cc @tqchen@junrushao1994@d-smirnov@masahi
junrushao
commented
Jun 2, 2021
I am supportive too, but we probably need to make it work (or have it disabled) on windows & macOS |
7fba3cb to
599cc0eCompareLunderberg
commented
Jun 2, 2021
Sounds good, and that is my goal. The MSVC/Windows side should work automatically with no extra flag, since its default is to give an error on undefined symbols. For MacOS, I've updated the check to pass |
c55aec6 to
b2b7ce8Compare…d symbols. If libtvm_runtime.so erroneously requires definitions that are only present in libtvm.so, the -Wl,--no-undefined flag forces them to be compile-time errors rather than runtime, and would be caught by the CI.
b2b7ce8 to
0415733CompareLunderberg
commented
Jun 4, 2021
The check now works on all systems. For MSVC, undefined symbols already result in an error, no extra flag is needed. For MacOS, the equivalent The check is applied only on libtvm.so and libtvm_runtime.so, and not to the more general |
| else() | ||
| set(TVM_NO_UNDEFINED_SYMBOLS "-Wl,--no-undefined") | ||
| endif() | ||
| message(STATUS "Forbidding undefined symbols in shared library, using ${TVM_NO_UNDEFINED_SYMBOLS} on platform ${CMAKE_SYSTEM_NAME}") |
There was a problem hiding this comment.
nit: this could be confusing to a user if BUILD_STATIC_RUNTIME is set
There was a problem hiding this comment.
Hmm, good point. The flag is applied to both libtvm.so and (if it is being built), libtvm_runtime.so, so I wouldn't want to drop the message entirely for a static runtime build. What are your thoughts on the following phrasing?
"Forbidding undefined symbols in shared libraries libtvm and (if applicable) libtvm_runtime, using ${TVM_NO_UNDEFINED_SYMBOLS} on platform ${CMAKE_SYSTEM_NAME}"
There was a problem hiding this comment.
oh oops, i meant to resolve this first but got trigger happy. i don't mind that phrasing, but since it applies in all cases, i think it's probably fine as-is too.
There was a problem hiding this comment.
Sounds good, and I'll keep it as is for now. Added an item to my todo list to update it later, but low priority since the as-is phrasing also works.
areusch
commented
Jun 4, 2021
@Lunderberg great! I think we can merge this, I have one nit that I'm not sure if you prefer to address. if you don't have cycles for that right now, i think we should just merge--the message does say "forbidding undefined symbols in shared library" |
junrushao
commented
Jun 4, 2021
@Lunderberg BTW, maybe out of the scope of this PR, just curious which libraries will have undefined symbols (other than libtvm and libtvm_runtime)? |
d-smirnov
left a comment
There was a problem hiding this comment.
Looks legitimate to me. Thanks @Lunderberg
Lunderberg
commented
Jun 4, 2021
@junrushao1994 I think it was the vta-hw build, though I'd have to tinker with it a bit to reproduce the failure. |
…d symbols. (apache#8178) If libtvm_runtime.so erroneously requires definitions that are only present in libtvm.so, the -Wl,--no-undefined flag forces them to be compile-time errors rather than runtime, and would be caught by the CI. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
…d symbols. (apache#8178) If libtvm_runtime.so erroneously requires definitions that are only present in libtvm.so, the -Wl,--no-undefined flag forces them to be compile-time errors rather than runtime, and would be caught by the CI. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
Adds
-Wl,-z,defsflag for linking libtvm.so and libtvm_runtime.so. While undefined symbols in libtvm.so would be caught by the CI as tests run, undefined symbols in libtvm_runtime.so (e.g. dependency onTargetintroduced in #8127, removed in #8171), would otherwise pass the CI.