[CMake] Add a linker test for -Bsymbolic-functions to AddLLVM #79539
Conversation
llvmbot
added
cmake
|
@llvm/pr-subscribers-clang Author: Brad Smith (brad0) ChangesAdd a linker test for -Bsymbolic-functions to AddLLVM and remove Full diff: https://github.com/llvm/llvm-project/pull/79539.diff 3 Files Affected:
diff --git a/clang/tools/clang-shlib/CMakeLists.txt b/clang/tools/clang-shlib/CMakeLists.txt
index 298d3a9d18fec8c..6afd3efebe53a0b 100644
--- a/clang/tools/clang-shlib/CMakeLists.txt
+++ b/clang/tools/clang-shlib/CMakeLists.txt
@@ -50,7 +50,7 @@ add_clang_library(clang-cpp
${_DEPS})
# Optimize function calls for default visibility definitions to avoid PLT and
# reduce dynamic relocations.
-if (NOT APPLE AND NOT MINGW AND NOT LLVM_LINKER_IS_SOLARISLD_ILLUMOS)
+if (NOT APPLE AND LINKER_SUPPORTS_B_SYMBOLIC_FUNCTIONS)
target_link_options(clang-cpp PRIVATE LINKER:-Bsymbolic-functions)
endif()
if (MINGW OR CYGWIN)
diff --git a/llvm/cmake/modules/AddLLVM.cmake b/llvm/cmake/modules/AddLLVM.cmake
index 5e9896185528246..a030489b6196023 100644
--- a/llvm/cmake/modules/AddLLVM.cmake
+++ b/llvm/cmake/modules/AddLLVM.cmake
@@ -241,12 +241,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 "")
@@ -260,6 +254,7 @@ if (NOT DEFINED LLVM_LINKER_DETECTED AND NOT WIN32)
endif()
function(add_link_opts target_name)
+ include(LLVMCheckLinkerFlag)
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.
@@ -291,7 +286,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(LLVMCheckLinkerFlag)
llvm_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
@@ -314,6 +308,11 @@ function(add_link_opts target_name)
set_property(TARGET ${target_name} APPEND_STRING PROPERTY
LINK_FLAGS " -Wl,-brtl")
endif()
+
+ if (NOT MINGW)
+ llvm_check_linker_flag(CXX "-Wl,-Bsymbolic-functions"
+ LINKER_SUPPORTS_B_SYMBOLIC_FUNCTIONS)
+ endif()
endfunction(add_link_opts)
# Set each output directory according to ${CMAKE_CONFIGURATION_TYPES}.
diff --git a/llvm/tools/llvm-shlib/CMakeLists.txt b/llvm/tools/llvm-shlib/CMakeLists.txt
index a47a0ec84c625c6..3e08dd5459a3f1a 100644
--- a/llvm/tools/llvm-shlib/CMakeLists.txt
+++ b/llvm/tools/llvm-shlib/CMakeLists.txt
@@ -49,7 +49,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 (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
|
brad0
force-pushed
the
cmake_symbolic_functions
branch
from
73eee52 to
024e972
Compare
What's the issue? GNU ld has had -Bsymbolic-functions since 2007 and for quite a few releases LLVM has been using |
This is binutils 2.17. We've had patches for awhile in our tree too specifically for the exceptions not using LLD. |
|
This would be nice for Cygwin as well. It seems GNU ld just silently ignores |
llvm/cmake/modules/AddLLVM.cmake
Outdated
| if (NOT MINGW) | ||
| llvm_check_linker_flag(CXX "-Wl,-Bsymbolic-functions" | ||
| LINKER_SUPPORTS_B_SYMBOLIC_FUNCTIONS) | ||
| endif() |
There was a problem hiding this comment.
This if is unnecessary thanks to this PR.
There was a problem hiding this comment.
I wasn't sure about that and left MinGW as is but it looks like it's just using either LLD or the binutils BFD linker depending on which combo compiler / toolchain you go with.
There was a problem hiding this comment.
BFD accepts -Bsymbolic-functions but it's a no-op for anything other than ELF. LLD rejects the option as unknown in the MinGW driver. So it would be fine to remove the if (NOT MINGW) wrapping that, and let it use the flag or not depending on whether the linker accepts it.
brad0
force-pushed
the
cmake_symbolic_functions
branch
4 times, most recently
from
e1d2ffe to
be6feeb
Compare
|
@rorth Can you please check this on Solaris? |
I've now tested the patch on while |
|
@rorth Thanks. It was more so the native linker I wasn't sure about. |
brad0
force-pushed
the
cmake_symbolic_functions
branch
from
be6feeb to
03859cb
Compare
| @@ -355,6 +349,9 @@ function(add_link_opts target_name) | |||
| set_property(TARGET ${target_name} APPEND_STRING PROPERTY | |||
| LINK_FLAGS " -Wl,-brtl") | |||
| endif() | |||
|
|
|||
| check_linker_flag(CXX "-Wl,-Bsymbolic-functions" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
Add a linker test for -Bsymbolic-functions to AddLLVM and remove the illumos hardcoded bits for its handling. OpenBSD also has a local patch for linking with the old BFD linker on mips64 and sparc64.
brad0
force-pushed
the
cmake_symbolic_functions
branch
from
03859cb to
97c82e6
Compare
|
Added a comment. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/202/builds/1313 Here is the relevant piece of the build log for the reference |
Add a linker test for -Bsymbolic-functions to AddLLVM and remove
the illumos hardcoded bits for its handling. OpenBSD also has a
local patch for linking with the old BFD linker on mips64 and
sparc64.