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

[libc++] Use -nostdlib++ on GCC unconditionally #68832

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Oct 11, 2023

We support GCC 13, which supports the flag. This allows simplifying the CMake logic around the use of -nostdlib++. Note that there are other places where we don't assume -nostdlib++ yet in the build, but this patch is intentionally trying to be small because this part of our CMake is pretty tricky.

@ldionne ldionne requested a review from a team as a code owner October 11, 2023 19:24
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 11, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 11, 2023

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

We support GCC 13, which supports the flag. This allows simplifying the CMake logic around the use of -nostdlib++. Note that there are other places where we don't assume -nostdlib++ yet in the build, but this patch is intentionally trying to be small because this part of our CMake is pretty tricky.


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

2 Files Affected:

  • (modified) libcxx/CMakeLists.txt (+4-29)
  • (modified) libcxx/cmake/config-ix.cmake (-15)
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 16540caf68eaf04..f36e2f13b37f350 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -642,18 +642,11 @@ get_sanitizer_flags(SANITIZER_FLAGS "${LLVM_USE_SANITIZER}")
 
 # Link system libraries =======================================================
 function(cxx_link_system_libraries target)
-
-# In order to remove just libc++ from the link step
-# we need to use -nostdlib++ whenever it is supported.
-# Unfortunately this cannot be used universally because for example g++ supports
-# only -nodefaultlibs in which case all libraries will be removed and
-# all libraries but c++ have to be added in manually.
-  if (CXX_SUPPORTS_NOSTDLIBXX_FLAG)
-    target_add_link_flags_if_supported(${target} PRIVATE "-nostdlib++")
+  if (MSVC)
+    target_add_compile_flags(${target} PRIVATE "/Zl")
+    target_add_link_flags(${target} PRIVATE "/nodefaultlib")
   else()
-    target_add_link_flags_if_supported(${target} PRIVATE "-nodefaultlibs")
-    target_add_compile_flags_if_supported(${target} PRIVATE "/Zl")
-    target_add_link_flags_if_supported(${target} PRIVATE "/nodefaultlib")
+    target_add_link_flags(${target} PRIVATE "-nostdlib++")
   endif()
 
   if (CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG AND LIBCXXABI_USE_LLVM_UNWINDER)
@@ -663,24 +656,6 @@ function(cxx_link_system_libraries target)
     target_add_link_flags_if_supported(${target} PRIVATE "--unwindlib=none")
   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_M_LIB)
-      target_link_libraries(${target} PRIVATE m)
-    endif()
-
-    if (LIBCXX_HAS_RT_LIB)
-      target_link_libraries(${target} PRIVATE rt)
-    endif()
-  endif()
-
   if (LIBCXX_USE_COMPILER_RT)
     find_compiler_rt_library(builtins LIBCXX_BUILTINS_LIBRARY)
     if (LIBCXX_BUILTINS_LIBRARY)
diff --git a/libcxx/cmake/config-ix.cmake b/libcxx/cmake/config-ix.cmake
index 9962d848d85e846..9fed861a4e193c5 100644
--- a/libcxx/cmake/config-ix.cmake
+++ b/libcxx/cmake/config-ix.cmake
@@ -14,14 +14,6 @@ include(CheckCSourceCompiles)
 # link with --uwnindlib=none. Check if that option works.
 llvm_check_compiler_linker_flag(C "--unwindlib=none" CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG)
 
-if(WIN32 AND NOT MINGW)
-  # NOTE(compnerd) this is technically a lie, there is msvcrt, but for now, lets
-  # let the default linking take care of that.
-  set(LIBCXX_HAS_C_LIB NO)
-else()
-  check_library_exists(c fopen "" LIBCXX_HAS_C_LIB)
-endif()
-
 if (NOT LIBCXX_USE_COMPILER_RT)
   if(WIN32 AND NOT MINGW)
     set(LIBCXX_HAS_GCC_S_LIB NO)
@@ -54,9 +46,6 @@ else()
 endif()
 
 if (CXX_SUPPORTS_NOSTDLIBXX_FLAG OR C_SUPPORTS_NODEFAULTLIBS_FLAG)
-  if (LIBCXX_HAS_C_LIB)
-    list(APPEND CMAKE_REQUIRED_LIBRARIES c)
-  endif ()
   if (LIBCXX_USE_COMPILER_RT)
     include(HandleCompilerRT)
     find_compiler_rt_library(builtins LIBCXX_BUILTINS_LIBRARY
@@ -108,22 +97,18 @@ if(WIN32 AND NOT MINGW)
   # TODO(compnerd) do we want to support an emulation layer that allows for the
   # use of pthread-win32 or similar libraries to emulate pthreads on Windows?
   set(LIBCXX_HAS_PTHREAD_LIB NO)
-  set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_RT_LIB NO)
   set(LIBCXX_HAS_ATOMIC_LIB NO)
 elseif(APPLE)
   set(LIBCXX_HAS_PTHREAD_LIB NO)
-  set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_RT_LIB NO)
   set(LIBCXX_HAS_ATOMIC_LIB NO)
 elseif(FUCHSIA)
-  set(LIBCXX_HAS_M_LIB NO)
   set(LIBCXX_HAS_PTHREAD_LIB NO)
   set(LIBCXX_HAS_RT_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)
   check_library_exists(atomic __atomic_fetch_add_8 "" LIBCXX_HAS_ATOMIC_LIB)
 endif()

We support GCC 13, which supports the flag. This allows simplifying
the CMake logic around the use of -nostdlib++. Note that there are
other places where we don't assume -nostdlib++ yet in the build, but
this patch is intentionally trying to be small because this part of
our CMake is pretty tricky.
@ldionne ldionne force-pushed the review/cmake-nostdlibxx branch from 505ccb7 to 9ca88d1 Compare October 12, 2023 05:15
Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

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

LGTM, but could you double check on libm before committing?

Then again, we have great CI coverage now, so if it builds ship it?

libcxx/cmake/config-ix.cmake Show resolved Hide resolved
libcxx/cmake/config-ix.cmake Show resolved Hide resolved
Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

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

It's nice having CI for all the platforms, otherwise this would be harder to review and be confident about.

LGTM assuming everything passes.

libcxx/cmake/config-ix.cmake Show resolved Hide resolved
libcxx/cmake/config-ix.cmake Show resolved Hide resolved
@ldionne ldionne merged commit 36bb134 into llvm:main Oct 13, 2023
@ldionne ldionne deleted the review/cmake-nostdlibxx branch October 13, 2023 18:54
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.

4 participants