-
Notifications
You must be signed in to change notification settings - Fork 15k
[libcxx] Finish localization support for LLVM libc #154170
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
base: main
Are you sure you want to change the base?
Conversation
This patch enables building libcxx with LIBCXX_ENABLE_LOCALIZATION=ON when building against LLVM libc. This finishes up llvm#97508.
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-libcxx Author: Aiden Grossman (boomanaiden154) ChangesThis patch enables building libcxx with LIBCXX_ENABLE_LOCALIZATION=ON when building against LLVM libc. This finishes up #97508. Full diff: https://github.com/llvm/llvm-project/pull/154170.diff 1 Files Affected:
diff --git a/libcxx/include/__locale_dir/messages.h b/libcxx/include/__locale_dir/messages.h
index c04bf04025ff0..1444b5b64529d 100644
--- a/libcxx/include/__locale_dir/messages.h
+++ b/libcxx/include/__locale_dir/messages.h
@@ -22,7 +22,7 @@
# if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__))
// Most unix variants have catopen. These are the specific ones that don't.
-# if !defined(__BIONIC__) && !defined(_NEWLIB_VERSION) && !defined(__EMSCRIPTEN__)
+# if !defined(__BIONIC__) && !defined(_NEWLIB_VERSION) && !defined(__EMSCRIPTEN__) && !defined(__LLVM_LIBC__)
# define _LIBCPP_HAS_CATOPEN 1
# include <nl_types.h>
# else
|
|
I know there were talks about getting CI set up for testing LLVM libc and libc++ in #97508. I'm assuming this is still desired? If so, I can work on getting a configuration setup that runs in stage 3. And assuming we still need CI, would that be a blocker for this patch or could we land this patch concurrently with CI work? |
I'm quite confident that you'll actually follow up on that and the patch is quite simple, so I don't think it's a blocker. It'd still be nice to get soon. I'm actually quite excited about it, since having some basic CI allow us to have a minimal libc configuration we can test. |
Sounds good. I'll start working on that.
I was thinking with the LLVM libc combination to test as much functionality as LLVM libc supports enabling and to incrementally increase that as necessary changes are made (big thing currently being better wchar support). That sounds like it would be separate from a minimal libc configuration though. I think that might already be exercised by the Fuchsia embedded scenario, but I'm not really sure. Thoughts? |
I'd really like multiple configurations. The most important one is the "everything LLVM libc supports", but I'd also like a minimal configuration that makes sure we don't rely on anything we don't want to. This is primarily for better testing of libc++, not LLVM libc. If LLVM libc supports all the stuff we need at some point I'd also like to switch the "libc++ with X disabled" configurations over so we actually cannot rely on the stuff we don't want to rely on. That would most likely allow us to simplify some stuff in libc++ and catch bugs earlier. |
Makes sense. I'll work on getting a patch up for the former. The latter shouldn't be too difficult either.
I'm assuming you mean switching those configurations over to test against libc, probably with the relevant entrypoints disabled so you can't accidentally use libc functionality that you would otherwise be able to just link against? |
d0da142 to
7b2137b
Compare
Yes, exactly. |
This patch enables building libcxx with LIBCXX_ENABLE_LOCALIZATION=ON when building against LLVM libc.
This finishes up #97508.