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.

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

[runtimes] Fix link order of system librarires on Apple platforms #66940

Merged

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Sep 20, 2023

On Apple platforms, we always support the -nostdlib++ flag. Hence, it is not necessary to manually link against system libraries. In fact, doing so causes us to link against libSystem explicitly, which messes up with the order of libraries we should use. Indeed:

   Before patch, using the system unwinder (LIBCXXABI_USE_LLVM_UNWINDER = OFF)
   ===========================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @rpath/libc++.1.dylib
         /usr/lib/libSystem.B.dylib
         @rpath/libc++abi.1.dylib
   lib/libc++abi.1.dylib:
         @rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   After patch, using the system unwinder (LIBCXXABI_USE_LLVM_UNWINDER = OFF)
   ===========================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @rpath/libc++.1.dylib
         @rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libc++abi.1.dylib:
         @rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   Before patch, with the LLVM unwinder (LIBCXXABI_USE_LLVM_UNWINDER = ON)
   =======================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @rpath/libc++.1.dylib
         /usr/lib/libSystem.B.dylib
         @rpath/libc++abi.1.dylib
         @rpath/libunwind.1.dylib
   lib/libc++abi.1.dylib:
         @rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
         @rpath/libunwind.1.dylib
   lib/libunwind.1.dylib:
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   After patch, with the LLVM unwinder (LIBCXXABI_USE_LLVM_UNWINDER = ON)
   ======================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @rpath/libc++.1.dylib
         @rpath/libc++abi.1.dylib
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libc++abi.1.dylib:
         @rpath/libc++abi.1.dylib
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

As we can see, libSystem appears before the just-built libraries before the patch, which causes the libunwind.dylib bundled in libSystem.dylib to be used instead of the just-built libunwind.dylib.

We didn't notice the issue until recently when I tried to update the macOS CI builders to macOS 13.5, where it is necessary to use the right libunwind library (the exact reason still needs to be investigated).

On Apple platforms, we always support the -nostdlib++ flag. Hence, it is
not necessary to manually link against system libraries. In fact, doing
so causes us to link against libSystem explicitly, which messes up with
the order of libraries we should use. Indeed:

   Before patch, using the system unwinder (LIBCXXABI_USE_LLVM_UNWINDER = OFF)
   ===========================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @rpath/libc++.1.dylib
         /usr/lib/libSystem.B.dylib
         @rpath/libc++abi.1.dylib
   lib/libc++abi.1.dylib:
         @rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   After patch, using the system unwinder (LIBCXXABI_USE_LLVM_UNWINDER = OFF)
   ===========================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @rpath/libc++.1.dylib
         @rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libc++abi.1.dylib:
         @rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   Before patch, with the LLVM unwinder (LIBCXXABI_USE_LLVM_UNWINDER = ON)
   =======================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @rpath/libc++.1.dylib
         /usr/lib/libSystem.B.dylib
         @rpath/libc++abi.1.dylib
         @rpath/libunwind.1.dylib
   lib/libc++abi.1.dylib:
         @rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
         @rpath/libunwind.1.dylib
   lib/libunwind.1.dylib:
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   After patch, with the LLVM unwinder (LIBCXXABI_USE_LLVM_UNWINDER = ON)
   ======================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @rpath/libc++.1.dylib
         @rpath/libc++abi.1.dylib
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libc++abi.1.dylib:
         @rpath/libc++abi.1.dylib
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

As we can see, libSystem appears before the just-built libraries before
the patch, which causes the libunwind.dylib bundled in libSystem.dylib
to be used instead of the just-built libunwind.dylib.

We didn't notice the issue until recently when I tried to update the
macOS CI builders to macOS 13.5, where it is necessary to use the
right libunwind library (the exact reason still needs to be investigated).
@ldionne ldionne requested review from a team as code owners September 20, 2023 18:44
@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libc++abi libc++abi C++ Runtime Library. Not libc++. libunwind labels Sep 20, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 20, 2023

@llvm/pr-subscribers-libunwind
@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libcxx

Changes

On Apple platforms, we always support the -nostdlib++ flag. Hence, it is not necessary to manually link against system libraries. In fact, doing so causes us to link against libSystem explicitly, which messes up with the order of libraries we should use. Indeed:

   Before patch, using the system unwinder (LIBCXXABI_USE_LLVM_UNWINDER = OFF)
   ===========================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @<!-- -->rpath/libc++.1.dylib
         /usr/lib/libSystem.B.dylib
         @<!-- -->rpath/libc++abi.1.dylib
   lib/libc++abi.1.dylib:
         @<!-- -->rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @<!-- -->rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   After patch, using the system unwinder (LIBCXXABI_USE_LLVM_UNWINDER = OFF)
   ===========================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @<!-- -->rpath/libc++.1.dylib
         @<!-- -->rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libc++abi.1.dylib:
         @<!-- -->rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @<!-- -->rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   Before patch, with the LLVM unwinder (LIBCXXABI_USE_LLVM_UNWINDER = ON)
   =======================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @<!-- -->rpath/libc++.1.dylib
         /usr/lib/libSystem.B.dylib
         @<!-- -->rpath/libc++abi.1.dylib
         @<!-- -->rpath/libunwind.1.dylib
   lib/libc++abi.1.dylib:
         @<!-- -->rpath/libc++abi.1.dylib
         /usr/lib/libSystem.B.dylib
         @<!-- -->rpath/libunwind.1.dylib
   lib/libunwind.1.dylib:
         @<!-- -->rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

   After patch, with the LLVM unwinder (LIBCXXABI_USE_LLVM_UNWINDER = ON)
   ======================================================================
   $ otool -L lib/{libc++.1.dylib,libc++abi.1.dylib,libunwind.1.dylib}
   lib/libc++.1.dylib:
         @<!-- -->rpath/libc++.1.dylib
         @<!-- -->rpath/libc++abi.1.dylib
         @<!-- -->rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libc++abi.1.dylib:
         @<!-- -->rpath/libc++abi.1.dylib
         @<!-- -->rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib
   lib/libunwind.1.dylib:
         @<!-- -->rpath/libunwind.1.dylib
         /usr/lib/libSystem.B.dylib

As we can see, libSystem appears before the just-built libraries before the patch, which causes the libunwind.dylib bundled in libSystem.dylib to be used instead of the just-built libunwind.dylib.

We didn't notice the issue until recently when I tried to update the macOS CI builders to macOS 13.5, where it is necessary to use the right libunwind library (the exact reason still needs to be investigated).


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

5 Files Affected:

  • (modified) libcxx/CMakeLists.txt (+13-15)
  • (modified) libcxx/cmake/config-ix.cmake (-4)
  • (modified) libcxxabi/cmake/config-ix.cmake (-2)
  • (modified) libcxxabi/src/CMakeLists.txt (+1-3)
  • (modified) libunwind/src/CMakeLists.txt (+10-3)
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index bb2898b799bcef9..68410feb9661816 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -659,24 +659,22 @@ function(cxx_link_system_libraries target)
     target_add_link_flags_if_supported(${target} PRIVATE "--unwindlib=none")
   endif()
 
-  if (LIBCXX_HAS_SYSTEM_LIB)
-    target_link_libraries(${target} PRIVATE System)
-  endif()
-
-  if (LIBCXX_HAS_PTHREAD_LIB)
-    target_link_libraries(${target} PRIVATE pthread)
-  endif()
+  if (NOT APPLE) # On Apple platforms, we always use -nostdlib++ so we don't need to re-add other libraries
+    if (LIBCXX_HAS_PTHREAD_LIB)
+      target_link_libraries(${target} PRIVATE pthread)
+    endif()
 
-  if (LIBCXX_HAS_C_LIB)
-    target_link_libraries(${target} PRIVATE c)
-  endif()
+    if (LIBCXX_HAS_C_LIB)
+      target_link_libraries(${target} PRIVATE c)
+    endif()
 
-  if (LIBCXX_HAS_M_LIB)
-    target_link_libraries(${target} PRIVATE m)
-  endif()
+    if (LIBCXX_HAS_M_LIB)
+      target_link_libraries(${target} PRIVATE m)
+    endif()
 
-  if (LIBCXX_HAS_RT_LIB)
-    target_link_libraries(${target} PRIVATE rt)
+    if (LIBCXX_HAS_RT_LIB)
+      target_link_libraries(${target} PRIVATE rt)
+    endif()
   endif()
 
   if (LIBCXX_USE_COMPILER_RT)
diff --git a/libcxx/cmake/config-ix.cmake b/libcxx/cmake/config-ix.cmake
index 3bae53643683581..9962d848d85e846 100644
--- a/libcxx/cmake/config-ix.cmake
+++ b/libcxx/cmake/config-ix.cmake
@@ -110,10 +110,8 @@ if(WIN32 AND NOT MINGW)
   set(LIBCXX_HAS_PTHREAD_LIB NO)
   set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_RT_LIB NO)
-  set(LIBCXX_HAS_SYSTEM_LIB NO)
   set(LIBCXX_HAS_ATOMIC_LIB NO)
 elseif(APPLE)
-  check_library_exists(System write "" LIBCXX_HAS_SYSTEM_LIB)
   set(LIBCXX_HAS_PTHREAD_LIB NO)
   set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_RT_LIB NO)
@@ -122,12 +120,10 @@ elseif(FUCHSIA)
   set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_PTHREAD_LIB NO)
   set(LIBCXX_HAS_RT_LIB NO)
-  set(LIBCXX_HAS_SYSTEM_LIB NO)
   check_library_exists(atomic __atomic_fetch_add_8 "" LIBCXX_HAS_ATOMIC_LIB)
 else()
   check_library_exists(pthread pthread_create "" LIBCXX_HAS_PTHREAD_LIB)
   check_library_exists(m ccos "" LIBCXX_HAS_M_LIB)
   check_library_exists(rt clock_gettime "" LIBCXX_HAS_RT_LIB)
-  set(LIBCXX_HAS_SYSTEM_LIB NO)
   check_library_exists(atomic __atomic_fetch_add_8 "" LIBCXX_HAS_ATOMIC_LIB)
 endif()
diff --git a/libcxxabi/cmake/config-ix.cmake b/libcxxabi/cmake/config-ix.cmake
index f4ee8946c1fea41..702fe7d1d72f777 100644
--- a/libcxxabi/cmake/config-ix.cmake
+++ b/libcxxabi/cmake/config-ix.cmake
@@ -95,11 +95,9 @@ if(FUCHSIA)
   set(LIBCXXABI_HAS_PTHREAD_LIB NO)
   check_library_exists(c __cxa_thread_atexit_impl ""
     LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
-  set(LIBCXXABI_HAS_SYSTEM_LIB NO)
 else()
   check_library_exists(dl dladdr "" LIBCXXABI_HAS_DL_LIB)
   check_library_exists(pthread pthread_once "" LIBCXXABI_HAS_PTHREAD_LIB)
   check_library_exists(c __cxa_thread_atexit_impl ""
     LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
-  check_library_exists(System write "" LIBCXXABI_HAS_SYSTEM_LIB)
 endif()
diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index 3ad755803ac9c87..a3a73620bd9ff78 100644
--- a/libcxxabi/src/CMakeLists.txt
+++ b/libcxxabi/src/CMakeLists.txt
@@ -67,9 +67,7 @@ if (LIBCXXABI_ENABLE_FORGIVING_DYNAMIC_CAST)
   add_definitions(-D_LIBCXXABI_FORGIVING_DYNAMIC_CAST)
 endif()
 
-if (APPLE)
-  add_library_flags_if(LIBCXXABI_HAS_SYSTEM_LIB System)
-else()
+if (NOT APPLE) # On Apple platforms, we always use -nostdlib++ so we don't need to re-add other libraries
   if (LIBCXXABI_ENABLE_THREADS)
     add_library_flags_if(LIBCXXABI_HAS_PTHREAD_LIB pthread)
   endif()
diff --git a/libunwind/src/CMakeLists.txt b/libunwind/src/CMakeLists.txt
index a9bf29a8b394a4c..452d988c3726b5e 100644
--- a/libunwind/src/CMakeLists.txt
+++ b/libunwind/src/CMakeLists.txt
@@ -65,16 +65,23 @@ set(LIBUNWIND_SOURCES
     ${LIBUNWIND_ASM_SOURCES})
 
 # Generate library list.
-add_library_flags_if(LIBUNWIND_HAS_C_LIB c)
 if (LIBUNWIND_USE_COMPILER_RT)
   add_library_flags("${LIBUNWIND_BUILTINS_LIBRARY}")
 else()
   add_library_flags_if(LIBUNWIND_HAS_GCC_S_LIB gcc_s)
   add_library_flags_if(LIBUNWIND_HAS_GCC_LIB gcc)
 endif()
-add_library_flags_if(LIBUNWIND_HAS_DL_LIB dl)
+if (NOT APPLE) # On Apple platforms, we don't need to link explicitly against system libraries
+  add_library_flags_if(LIBUNWIND_HAS_C_LIB c)
+  add_library_flags_if(LIBUNWIND_HAS_DL_LIB dl)
+
+  if (LIBUNWIND_ENABLE_THREADS)
+    add_library_flags_if(LIBUNWIND_HAS_PTHREAD_LIB pthread)
+    add_compile_flags_if(LIBUNWIND_WEAK_PTHREAD_LIB -DLIBUNWIND_USE_WEAK_PTHREAD=1)
+  endif()
+endif()
+
 if (LIBUNWIND_ENABLE_THREADS)
-  add_library_flags_if(LIBUNWIND_HAS_PTHREAD_LIB pthread)
   add_compile_flags_if(LIBUNWIND_WEAK_PTHREAD_LIB -DLIBUNWIND_USE_WEAK_PTHREAD=1)
 endif()
 

@ldionne ldionne merged commit e46de4e into llvm:main Sep 21, 2023
4 checks passed
@ldionne ldionne deleted the review/dont-link-against-system-libs-apple branch September 21, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++. libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libunwind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants