You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/06/02 15:54:08 UTC
[GitHub] [tvm] Lunderberg opened a new pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Lunderberg opened a new pull request #8178:
URL: https://github.com/apache/tvm/pull/8178
Adds `-Wl,-z,defs` flag 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 on `Target` introduced in #8127, removed in #8171), would otherwise pass the CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#issuecomment-854907874
@junrushao1994 I think it was the vta-hw build, though I'd have to tinker with it a bit to reproduce the failure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#discussion_r645864425
##########
File path: CMakeLists.txt
##########
@@ -165,6 +169,16 @@ else(MSVC)
set(CMAKE_CXX_FLAGS "-faligned-new ${CMAKE_CXX_FLAGS}")
endif()
+ # ld option to warn if symbols are undefined (e.g. libtvm_runtime.so
+ # using symbols only present in libtvm.so). Not needed for MSVC,
+ # since this is already the default there.
+ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
+ set(TVM_NO_UNDEFINED_SYMBOLS "-Wl,-undefined,error")
+ 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}")
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#issuecomment-853302658
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 `-undefined,error` on OSX platforms, and will keep an eye on the CI to make sure that it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#issuecomment-853144762
Potential reviewers: @areusch as this impacts the CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch merged pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #8178:
URL: https://github.com/apache/tvm/pull/8178
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#issuecomment-854880571
@Lunderberg BTW, maybe out of the scope of this PR, just curious which libraries will have undefined symbols (other than libtvm and libtvm_runtime)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#discussion_r645835949
##########
File path: CMakeLists.txt
##########
@@ -165,6 +169,16 @@ else(MSVC)
set(CMAKE_CXX_FLAGS "-faligned-new ${CMAKE_CXX_FLAGS}")
endif()
+ # ld option to warn if symbols are undefined (e.g. libtvm_runtime.so
+ # using symbols only present in libtvm.so). Not needed for MSVC,
+ # since this is already the default there.
+ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
+ set(TVM_NO_UNDEFINED_SYMBOLS "-Wl,-undefined,error")
+ 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}")
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#discussion_r645718355
##########
File path: CMakeLists.txt
##########
@@ -165,6 +169,16 @@ else(MSVC)
set(CMAKE_CXX_FLAGS "-faligned-new ${CMAKE_CXX_FLAGS}")
endif()
+ # ld option to warn if symbols are undefined (e.g. libtvm_runtime.so
+ # using symbols only present in libtvm.so). Not needed for MSVC,
+ # since this is already the default there.
+ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
+ set(TVM_NO_UNDEFINED_SYMBOLS "-Wl,-undefined,error")
+ 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}")
Review comment:
nit: this could be confusing to a user if BUILD_STATIC_RUNTIME is set
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#issuecomment-854873101
@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"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a change in pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on a change in pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#discussion_r645724658
##########
File path: CMakeLists.txt
##########
@@ -165,6 +169,16 @@ else(MSVC)
set(CMAKE_CXX_FLAGS "-faligned-new ${CMAKE_CXX_FLAGS}")
endif()
+ # ld option to warn if symbols are undefined (e.g. libtvm_runtime.so
+ # using symbols only present in libtvm.so). Not needed for MSVC,
+ # since this is already the default there.
+ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
+ set(TVM_NO_UNDEFINED_SYMBOLS "-Wl,-undefined,error")
+ 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}")
Review 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}"`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#issuecomment-853229325
I am supportive too, but we probably need to make it work (or have it disabled) on windows & macOS
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #8178: [CMake] Add compile-time check that .so files have no undefined symbols
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #8178:
URL: https://github.com/apache/tvm/pull/8178#issuecomment-854818470
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 `-Wl,-undefined,error` flag is used. I also switched to the equivalent but more readable `-Wl,--no-undefined` flag on linux.
The check is applied only on libtvm.so and libtvm_runtime.so, and not to the more general `CMAKE_SHARED_LINKER_FLAGS`, because some of the other libraries have undefined symbols that are resolved when linking against the tvm libraries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org