Skip to content

[libc++] Update a comment about -nostdlib++ #67429

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

Closed
wants to merge 1 commit into from

Conversation

nico
Copy link
Contributor

@nico nico commented Sep 26, 2023

GCC added support for the flag in gcc 13.1.

No behavior change.

GCC added support for the flag in gcc 13.1.

No behavior change.
@nico nico requested a review from a team as a code owner September 26, 2023 13:31
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 26, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 26, 2023

@llvm/pr-subscribers-libcxx

Changes

GCC added support for the flag in gcc 13.1.

No behavior change.


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

1 Files Affected:

  • (modified) libcxx/CMakeLists.txt (+4-2)
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index b9d0ed51be26033..c42337fa83062f7 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -624,9 +624,11 @@ 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
+# Unfortunately this cannot be used universally because g++ does not support
+# -nostdlib++ before gcc 13 (https://gcc.gnu.org/gcc-13/changes.html) and needs
+# to use -nodefaultlibs in which case all libraries will be removed and
 # all libraries but c++ have to be added in manually.
+# Once libc++ requires gcc 13+, this can be simplified.
   if (CXX_SUPPORTS_NOSTDLIBXX_FLAG)
     target_add_link_flags_if_supported(${target} PRIVATE "-nostdlib++")
   else()

@philnik777
Copy link
Contributor

libc++ already requires GCC13, so this can be simplified now.

@ldionne
Copy link
Member

ldionne commented Sep 27, 2023

libc++ already requires GCC13, so this can be simplified now.

I think the real reason why we're still checking for whether -nostdlib++ is supported is to handle Clang-cl, which doesn't support it IIUC. I think we could probably simplify that to be something like

if (MSVC)
  target_add_link_flags(${target} PRIVATE "/nodefaultlibs")
else()
  target_add_link_flags(${target} PRIVATE "-nostdlib++")
endif()

but then the rest of the function where we link against various system libraries will have to change too (we could drop a bunch of complexity on non-MSVC envs). @nico Do you want to go for that or do you want us to pick that up?

@nico
Copy link
Contributor Author

nico commented Oct 11, 2023

Feel free to pick it up, thanks!

@nico nico closed this Oct 11, 2023
@nico nico deleted the libcxx-gcc-13-comment branch October 11, 2023 02:11
@ldionne
Copy link
Member

ldionne commented Oct 11, 2023

@nico Here's my attempt to clean this up a bit further: #68832

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