Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CMake] Add compile-time check that .so files have no undefined symbols #8178

Merged
merged 1 commit into from
Jun 4, 2021
Merged

[CMake] Add compile-time check that .so files have no undefined symbols #8178

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ if(MSVC)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /EHsc")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /bigobj")

# MSVC already errors on undefined symbols, no additional flag needed.
set(TVM_NO_UNDEFINED_SYMBOLS "")

if(USE_MSVC_MT)
foreach(flag_var
CMAKE_CXX_FLAGS CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE
Expand Down Expand Up @@ -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}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this could be confusing to a user if BUILD_STATIC_RUNTIME is set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Detect if we're compiling for Hexagon.
set(TEST_FOR_HEXAGON_CXX
"#ifndef __hexagon__"
Expand Down Expand Up @@ -414,6 +428,7 @@ add_library(tvm_objs OBJECT ${COMPILER_SRCS})
add_library(tvm_runtime_objs OBJECT ${RUNTIME_SRCS})

add_library(tvm SHARED $<TARGET_OBJECTS:tvm_objs> $<TARGET_OBJECTS:tvm_runtime_objs>)
set_property(TARGET tvm APPEND PROPERTY LINK_OPTIONS "${TVM_NO_UNDEFINED_SYMBOLS}")
set_property(TARGET tvm APPEND PROPERTY LINK_OPTIONS "${TVM_VISIBILITY_FLAG}")
if(BUILD_STATIC_RUNTIME)
add_library(tvm_runtime STATIC $<TARGET_OBJECTS:tvm_runtime_objs>)
Expand All @@ -425,6 +440,7 @@ if(BUILD_STATIC_RUNTIME)
COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --yellow --bold ${NOTICE})
else()
add_library(tvm_runtime SHARED $<TARGET_OBJECTS:tvm_runtime_objs>)
set_property(TARGET tvm_runtime APPEND PROPERTY LINK_OPTIONS "${TVM_NO_UNDEFINED_SYMBOLS}")
endif()
set_property(TARGET tvm_runtime APPEND PROPERTY LINK_OPTIONS "${TVM_VISIBILITY_FLAG}")
target_compile_definitions(tvm_objs PUBLIC DMLC_USE_LOGGING_LIBRARY=<tvm/runtime/logging.h>)
Expand Down