Skip to content

[CMake] Add a linker test for -Bsymbolic-functions to AddLLVM #79539

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

Merged
merged 1 commit into from
May 19, 2025
+9 −9
Merged

[CMake] Add a linker test for -Bsymbolic-functions to AddLLVM #79539

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion clang/tools/clang-shlib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ endif()

# Optimize function calls for default visibility definitions to avoid PLT and
# reduce dynamic relocations.
if (NOT APPLE AND NOT MINGW AND NOT CYGWIN AND NOT LLVM_LINKER_IS_SOLARISLD_ILLUMOS)
if (NOT APPLE AND LLVM_LINKER_SUPPORTS_B_SYMBOLIC_FUNCTIONS)
target_link_options(clang-cpp PRIVATE LINKER:-Bsymbolic-functions)
endif()
if (MINGW OR CYGWIN)
Expand Down
14 changes: 7 additions & 7 deletions llvm/cmake/modules/AddLLVM.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,12 +257,6 @@ if (NOT DEFINED LLVM_LINKER_DETECTED AND NOT WIN32)
set(LLVM_LINKER_DETECTED YES CACHE INTERNAL "")
set(LLVM_LINKER_IS_GNULD YES CACHE INTERNAL "")
message(STATUS "Linker detection: GNU ld")
elseif("${stderr}" MATCHES "(illumos)" OR
"${stdout}" MATCHES "(illumos)")
set(LLVM_LINKER_DETECTED YES CACHE INTERNAL "")
set(LLVM_LINKER_IS_SOLARISLD YES CACHE INTERNAL "")
set(LLVM_LINKER_IS_SOLARISLD_ILLUMOS YES CACHE INTERNAL "")
message(STATUS "Linker detection: Solaris ld (illumos)")
elseif("${stderr}" MATCHES "Solaris Link Editors" OR
"${stdout}" MATCHES "Solaris Link Editors")
set(LLVM_LINKER_DETECTED YES CACHE INTERNAL "")
Expand Down Expand Up @@ -296,6 +290,7 @@ if (NOT DEFINED LLVM_LINKER_DETECTED AND NOT WIN32)
endif()

function(add_link_opts target_name)
include(CheckLinkerFlag)
get_llvm_distribution(${target_name} in_distribution in_distribution_var)
if(NOT in_distribution)
# Don't LTO optimize targets that aren't part of any distribution.
Expand Down Expand Up @@ -327,7 +322,6 @@ function(add_link_opts target_name)
elseif(${CMAKE_SYSTEM_NAME} MATCHES "SunOS" AND LLVM_LINKER_IS_SOLARISLD)
# Support for ld -z discard-unused=sections was only added in
# Solaris 11.4. GNU ld ignores it, but warns every time.
include(CheckLinkerFlag)
check_linker_flag(CXX "-Wl,-z,discard-unused=sections" LINKER_SUPPORTS_Z_DISCARD_UNUSED)
if (LINKER_SUPPORTS_Z_DISCARD_UNUSED)
set_property(TARGET ${target_name} APPEND_STRING PROPERTY
Expand Down Expand Up @@ -355,6 +349,12 @@ function(add_link_opts target_name)
set_property(TARGET ${target_name} APPEND_STRING PROPERTY
LINK_FLAGS " -Wl,-brtl")
endif()

# Check for existence of symbolic functions flag. Not supported
# by the older BFD linker (such as on some OpenBSD archs), the
# MinGW driver for LLD, and the Solaris native linker.
check_linker_flag(CXX "-Wl,-Bsymbolic-functions"
Copy link
Member

@MaskRay MaskRay May 15, 2025

Choose a reason for hiding this comment

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

Add a comment what targets may need the -Wl,-Bsymbolic-functions check?

While configure is right, llvm cmake is so complex and so slow that many contributors want to optimize the number of compile/linke checks. I would still hope that we only do this on targets that actually need it.

Copy link
Collaborator

@rorth rorth May 15, 2025

Choose a reason for hiding this comment

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

Please don't go this route: it makes feature tests completely unmaintainable. Consider the Solaris case where initially only /bin/ld was supported which didn't accept -Bsymbolic-functions. Only later when GNU ld became an alternative linker that one would support that option. How on earth is one supposed to know that I now have to revise the cmake check to detect this if it were restricted to some platforms?

Copy link
Member

@MaskRay MaskRay May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While I am aware of https://ewontfix.com/13/ ("Incorrect configure checks for availability of functions"), there have been many cmake changes to skip some checks that are guaranteed to succeed, to make cmake run faster (llvm's cmake is really slow; in the unofficial build system bazel, checks are also often hard-coded). I wonder whether there is a condition that can skip the check for systems that always have -Bsymbolic-functions.

Copy link
Contributor

@mati865 mati865 May 16, 2025

Choose a reason for hiding this comment

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

I'd argue it's much faster than autotools for other large projects. It would be nice if the configuration phase could be parallelized, at least to some degree, but even with a single thread, it's a matter of seconds.

Copy link
Member

@MaskRay MaskRay May 19, 2025

Choose a reason for hiding this comment

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

@mati865 This might be true for other, smaller projects, but for LLVM we have to be careful introducing new checks. https://discourse.llvm.org/t/cmake-compiler-flag-checks-are-really-slow-ideas-to-speed-them-up/78882
It could be slower on a slow device like NFS. For this single check_linker_flag it might be fine.

Copy link
Contributor

@christopherbate christopherbate Oct 4, 2025

Choose a reason for hiding this comment

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

Why was this check_linker_flag put inside add_linker_opts instead of at the call site where it is actually used in the llvm-shlib target? This function is called every time LLVM creates a library. Even after the first call when the cache variable is populated, this function seems to have significant overhead.

LLVM_LINKER_SUPPORTS_B_SYMBOLIC_FUNCTIONS)
endfunction(add_link_opts)

# Set each output directory according to ${CMAKE_CONFIGURATION_TYPES}.
Expand Down
2 changes: 1 addition & 1 deletion llvm/tools/llvm-shlib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ if(LLVM_BUILD_LLVM_DYLIB)
# Solaris ld does not accept global: *; so there is no way to version *all* global symbols
set(LIB_NAMES -Wl,--version-script,${LLVM_LIBRARY_DIR}/tools/llvm-shlib/simple_version_script.map ${LIB_NAMES})
endif()
if (NOT MINGW AND NOT LLVM_LINKER_IS_SOLARISLD_ILLUMOS)
if (LLVM_LINKER_SUPPORTS_B_SYMBOLIC_FUNCTIONS)
# Optimize function calls for default visibility definitions to avoid PLT and
# reduce dynamic relocations.
# Note: for -fno-pic default, the address of a function may be different from
Expand Down