-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libcxx] Define _LIBCPP_HAS_C8RTOMB_MBRTOC8
to true if compiling with clang
#152724
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?
[libcxx] Define _LIBCPP_HAS_C8RTOMB_MBRTOC8
to true if compiling with clang
#152724
Conversation
Starting from picolibc 1.8.9, the `char8_t` related functions are provided. This patch adds logic to detect the underlying picolibc version and define the `_LIBCPP_HAS_C8RTOMB_MBRTOC8 macro` accordingly.
@llvm/pr-subscribers-libcxx Author: Victor Campos (vhscampos) ChangesStarting from picolibc 1.8.9, the This patch adds logic to detect the underlying picolibc version and define the Full diff: https://github.com/llvm/llvm-project/pull/152724.diff 2 Files Affected:
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 77a71b6cf1cae..ab87ed4787b84 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -1027,8 +1027,12 @@ typedef __char32_t char32_t;
# else
# define _LIBCPP_HAS_C8RTOMB_MBRTOC8 0
# endif
-# else
-# define _LIBCPP_HAS_C8RTOMB_MBRTOC8 0
+# elif defined (_LIBCPP_PICOLIBC_PREREQ)
+# if _LIBCPP_PICOLIBC_PREREQ(1, 8, 9) && defined(__cpp_char8_t)
+# define _LIBCPP_HAS_C8RTOMB_MBRTOC8 1
+# else
+# define _LIBCPP_HAS_C8RTOMB_MBRTOC8 0
+# endif
# endif
// There are a handful of public standard library types that are intended to
diff --git a/libcxx/include/__configuration/platform.h b/libcxx/include/__configuration/platform.h
index f3c199dee172b..4f581a352038f 100644
--- a/libcxx/include/__configuration/platform.h
+++ b/libcxx/include/__configuration/platform.h
@@ -47,6 +47,10 @@
// user code. Move code paths that need _NEWLIB_VERSION to another customization mechanism.
#if __has_include(<picolibc.h>)
# include <picolibc.h>
+# define _LIBCPP_PICOLIBC_VERSION_INT(maj, min, patch) (maj * 10000 + min * 100 + patch)
+# define _LIBCPP_PICOLIBC_PREREQ(maj, min, patch) \
+ _LIBCPP_PICOLIBC_VERSION_INT(__PICOLIBC__, __PICOLIBC_MINOR__, __PICOLIBC_PATCHLEVEL__) >= \
+ _LIBCPP_PICOLIBC_VERSION_INT(maj, min, patch)
#endif
#ifndef __BYTE_ORDER__
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
PR llvm/llvm-project#152724 adds logic to define the `_LIBCPP_HAS_C8RTOMB_MBRTOC8` macro depending on the picolibc support for `char8_t` functions. With this change we don't need a downstream patch anymore.
PR llvm/llvm-project#152724 adds logic to define the `_LIBCPP_HAS_C8RTOMB_MBRTOC8` macro depending on the picolibc support for `char8_t` functions. With this change we don't need a downstream patch anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to refactor _LIBCPP_HAS_C8RTOMB_MBRTOC8
so that it's true unless we know it to be false (and also unconditionally make it true with Clang).
@philnik777 I don't understand the two suggestions you made. Can you please give more details? Thank you |
|
I've refactored in a way that the macro is true "by default" as you suggested. However, with regards to always setting it to true if compiling with clang, that causes a libcxx test to fail (link:
The value of macro
I couldn't think of a way to modify the test in order to have the macro true for clang. |
We should probably just XFAIL the test on platforms that haven't fully implemented the interface. @ldionne any thoughts? |
Handling every case is brittle because I can't run the CI jobs locally, e.g. on AIX. Sorry for the high traffic. |
@philnik777 Can you please have another look? |
- Define macro unconditionally when the compiler is clang. - Remove preprocessor logic that isn't needed anymore. - Define new libcxx test feature for the AIX platform. - Mark tests as xfail in the platforms whose C library doesn't support the char8_t functions.
@philnik777 I've changed it around to unconditionally define the macro for clang, and then xfail tests in platforms whose C library doesn't provide support. The CI premerge failures AFAIK are unrelated to the patch. |
_LIBCPP_HAS_C8RTOMB_MBRTOC8
to true if compiling with clang
libcxx/test/std/strings/c.strings/no_c8rtomb_mbrtoc8.verify.cpp
Outdated
Show resolved
Hide resolved
- Move clang's case to the top. - Extract char8_t part of the tests into its own files. - Remove test that has little use. - Undo the creation of a feature just for the AIX platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nit.
libcxx/test/std/depr/depr.c.headers/uchar_h_char8_t.compile.pass.cpp
Outdated
Show resolved
Hide resolved
@philnik777 @ldionne
I don't know how to specify xfails to capture the conditions of these failures, or if it's even possible. If I xfail it on Linux across the board, I think it will unexpectedly pass on downstream runners that have a newer glibc. I'm lost here. Do you have any recommendations to make progress on this? |
If you can write out in pseudocode what your ideal XFAIL would be, I can try to figure it out. I've spent a lot of time in these test frameworks, for better or worse. |
For inspecting glibc,
glibc_version_less_than that could help.
But I get the impression it has to be more than just "glibc >= some verison". |
Define
_LIBCPP_HAS_C8RTOMB_MBRTOC8
to1
if compiling with clang.Some tests involving functionality from
uchar.h
/cuchar
fail when the platform does not provide support for the corresponding features. These have been xfailed on the relevant platforms.This patch will enable the adoption of newer picolibc versions.