Skip to content

[libc++] Simplify the conditions for generating a linker script #73151

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.

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
Nov 23, 2023

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Nov 22, 2023

We really shouldn't be depending on far away configuration options like LLVM_HAVE_LINK_VERSION_SCRIPT here. This patch simplifies the enablement of the linker scripts and as a result gets rid of an undesirable dependency on HandleLLVMOptions.cmake.

As a drive-by, the patch also stops taking into account whether Python3 is available. This should have no bearing on whether we generate a linker script or not, which is required for correctness. If someone tries to build libc++ and generate a linker script but Python3 is not available, they should get an error instead of silently getting an incorrect installation of the library.

We really shouldn't be depending on far away configuration options
like LLVM_HAVE_LINK_VERSION_SCRIPT here. This patch simplifies the
enablement of the linker scripts and as a result gets rid of an
undesirable dependency on HandleLLVMOptions.cmake.

As a drive-by, the patch also stops taking into account whether Python3
is available. This should have no bearing on whether we generate a linker
script or not, which is required for correctness. If someone tries to
build libc++ and generate a linker script but Python3 is not available,
they should get an error instead of silently getting an incorrect
installation of the library.
@ldionne ldionne requested a review from a team as a code owner November 22, 2023 17:57
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 22, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 22, 2023

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

We really shouldn't be depending on far away configuration options like LLVM_HAVE_LINK_VERSION_SCRIPT here. This patch simplifies the enablement of the linker scripts and as a result gets rid of an undesirable dependency on HandleLLVMOptions.cmake.

As a drive-by, the patch also stops taking into account whether Python3 is available. This should have no bearing on whether we generate a linker script or not, which is required for correctness. If someone tries to build libc++ and generate a linker script but Python3 is not available, they should get an error instead of silently getting an incorrect installation of the library.


Full diff: https://github.com/llvm/llvm-project/pull/73151.diff

1 Files Affected:

  • (modified) libcxx/CMakeLists.txt (+11-10)
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index cb708baacd91dec..843ccbd0ed92edb 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -251,17 +251,18 @@ option(LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY
 
 # Generate and install a linker script inplace of libc++.so. The linker script
 # will link libc++ to the correct ABI library. This option is on by default
-# on UNIX platforms other than Apple unless
-# 'LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY' is on. This option is also
-# disabled when the ABI library is not specified or is specified to be "none".
-set(ENABLE_LINKER_SCRIPT_DEFAULT_VALUE OFF)
-if (LLVM_HAVE_LINK_VERSION_SCRIPT AND NOT LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY
-      AND NOT LIBCXX_CXX_ABI STREQUAL "none"
-      AND Python3_EXECUTABLE
-      AND LIBCXX_ENABLE_SHARED)
-    set(ENABLE_LINKER_SCRIPT_DEFAULT_VALUE ON)
+# on UNIX platforms other than Apple unless we statically link libc++abi
+# inside libc++.so, we don't build libc++.so at all or we don't have any
+# ABI library.
+if (LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY
+    OR NOT LIBCXX_ENABLE_SHARED
+    OR LIBCXX_CXX_ABI STREQUAL "none")
+  set(ENABLE_LINKER_SCRIPT_DEFAULT_VALUE OFF)
+elseif(UNIX AND NOT APPLE)
+  set(ENABLE_LINKER_SCRIPT_DEFAULT_VALUE ON)
+else()
+  set(ENABLE_LINKER_SCRIPT_DEFAULT_VALUE OFF)
 endif()
-
 option(LIBCXX_ENABLE_ABI_LINKER_SCRIPT
       "Use and install a linker script for the given ABI library"
       ${ENABLE_LINKER_SCRIPT_DEFAULT_VALUE})

@ldionne ldionne merged commit 4b1fe09 into llvm:main Nov 23, 2023
@ldionne ldionne deleted the review/fix-linker-script-conditions branch November 23, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants