Skip to content

[libc++] Add __iswctype to the locale base API since it's required by <locale> #122168

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

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Jan 8, 2025

No description provided.

@ldionne ldionne requested a review from a team as a code owner January 8, 2025 20:49
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 8, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 8, 2025

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

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

4 Files Affected:

  • (modified) libcxx/include/__locale_dir/locale_base_api.h (+4)
  • (modified) libcxx/include/__locale_dir/support/bsd_like.h (+4)
  • (modified) libcxx/include/__locale_dir/support/windows.h (+3)
  • (modified) libcxx/src/locale.cpp (+5-5)
diff --git a/libcxx/include/__locale_dir/locale_base_api.h b/libcxx/include/__locale_dir/locale_base_api.h
index c8097beb9052df..6e3392e2dce8b8 100644
--- a/libcxx/include/__locale_dir/locale_base_api.h
+++ b/libcxx/include/__locale_dir/locale_base_api.h
@@ -56,6 +56,7 @@
 //  int     __strcoll(const char*, const char*, __locale_t);
 //  size_t  __strxfrm(char*, const char*, size_t, __locale_t);
 //
+//  int     __iswctype(wint_t, wctype_t, __locale_t);
 //  int     __iswspace(wint_t, __locale_t);
 //  int     __iswprint(wint_t, __locale_t);
 //  int     __iswcntrl(wint_t, __locale_t);
@@ -192,6 +193,9 @@ inline _LIBCPP_HIDE_FROM_ABI int __wcscoll(const wchar_t* __s1, const wchar_t* _
 inline _LIBCPP_HIDE_FROM_ABI size_t __wcsxfrm(wchar_t* __dest, const wchar_t* __src, size_t __n, __locale_t __loc) {
   return wcsxfrm_l(__dest, __src, __n, __loc);
 }
+inline _LIBCPP_HIDE_FROM_ABI int __iswctype(wint_t __ch, wctype_t __type, __locale_t __loc) {
+  return iswctype_l(__ch, __type, __loc);
+}
 inline _LIBCPP_HIDE_FROM_ABI int __iswspace(wint_t __ch, __locale_t __loc) { return iswspace_l(__ch, __loc); }
 inline _LIBCPP_HIDE_FROM_ABI int __iswprint(wint_t __ch, __locale_t __loc) { return iswprint_l(__ch, __loc); }
 inline _LIBCPP_HIDE_FROM_ABI int __iswcntrl(wint_t __ch, __locale_t __loc) { return iswcntrl_l(__ch, __loc); }
diff --git a/libcxx/include/__locale_dir/support/bsd_like.h b/libcxx/include/__locale_dir/support/bsd_like.h
index da31aeaf3c58e3..b3933c71c6b26d 100644
--- a/libcxx/include/__locale_dir/support/bsd_like.h
+++ b/libcxx/include/__locale_dir/support/bsd_like.h
@@ -94,6 +94,10 @@ inline _LIBCPP_HIDE_FROM_ABI size_t __strxfrm(char* __dest, const char* __src, s
 }
 
 #if _LIBCPP_HAS_WIDE_CHARACTERS
+inline _LIBCPP_HIDE_FROM_ABI int __iswctype(wint_t __c, wctype_t __type, __locale_t __loc) {
+  return ::iswctype_l(__c, __type, __loc);
+}
+
 inline _LIBCPP_HIDE_FROM_ABI int __iswspace(wint_t __c, __locale_t __loc) { return ::iswspace_l(__c, __loc); }
 
 inline _LIBCPP_HIDE_FROM_ABI int __iswprint(wint_t __c, __locale_t __loc) { return ::iswprint_l(__c, __loc); }
diff --git a/libcxx/include/__locale_dir/support/windows.h b/libcxx/include/__locale_dir/support/windows.h
index 03d05a410fdc3b..eca0e17d94c85a 100644
--- a/libcxx/include/__locale_dir/support/windows.h
+++ b/libcxx/include/__locale_dir/support/windows.h
@@ -206,6 +206,9 @@ inline _LIBCPP_HIDE_FROM_ABI size_t __strxfrm(char* __dest, const char* __src, s
 }
 
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
+inline _LIBCPP_HIDE_FROM_ABI int __iswctype(wint_t __c, wctype_t __type, __locale_t __loc) {
+  return ::_iswctype_l(__c, __type, __loc);
+}
 inline _LIBCPP_HIDE_FROM_ABI int __iswspace(wint_t __c, __locale_t __loc) { return ::_iswspace_l(__c, __loc); }
 inline _LIBCPP_HIDE_FROM_ABI int __iswprint(wint_t __c, __locale_t __loc) { return ::_iswprint_l(__c, __loc); }
 inline _LIBCPP_HIDE_FROM_ABI int __iswcntrl(wint_t __c, __locale_t __loc) { return ::_iswcntrl_l(__c, __loc); }
diff --git a/libcxx/src/locale.cpp b/libcxx/src/locale.cpp
index a1e10401f0b299..599c61ca7a36da 100644
--- a/libcxx/src/locale.cpp
+++ b/libcxx/src/locale.cpp
@@ -1109,11 +1109,11 @@ ctype_byname<wchar_t>::ctype_byname(const string& name, size_t refs)
 ctype_byname<wchar_t>::~ctype_byname() { __locale::__freelocale(__l_); }
 
 bool ctype_byname<wchar_t>::do_is(mask m, char_type c) const {
+  wint_t ch = static_cast<wint_t>(c);
 #  ifdef _LIBCPP_WCTYPE_IS_MASK
-  return static_cast<bool>(iswctype_l(c, m, __l_));
+  return static_cast<bool>(__locale::__iswctype(ch, m, __l_));
 #  else
   bool result = false;
-  wint_t ch   = static_cast<wint_t>(c);
   if ((m & space) == space)
     result |= (__locale::__iswspace(ch, __l_) != 0);
   if ((m & print) == print)
@@ -1179,7 +1179,7 @@ const wchar_t* ctype_byname<wchar_t>::do_is(const char_type* low, const char_typ
 const wchar_t* ctype_byname<wchar_t>::do_scan_is(mask m, const char_type* low, const char_type* high) const {
   for (; low != high; ++low) {
 #  ifdef _LIBCPP_WCTYPE_IS_MASK
-    if (iswctype_l(*low, m, __l_))
+    if (__locale::__iswctype(static_cast<wint_t>(*low), m, __l_))
       break;
 #  else
     wint_t ch = static_cast<wint_t>(*low);
@@ -1210,11 +1210,11 @@ const wchar_t* ctype_byname<wchar_t>::do_scan_is(mask m, const char_type* low, c
 
 const wchar_t* ctype_byname<wchar_t>::do_scan_not(mask m, const char_type* low, const char_type* high) const {
   for (; low != high; ++low) {
+    wint_t ch = static_cast<wint_t>(*low);
 #  ifdef _LIBCPP_WCTYPE_IS_MASK
-    if (!iswctype_l(*low, m, __l_))
+    if (!__locale::__iswctype(ch, m, __l_))
       break;
 #  else
-    wint_t ch = static_cast<wint_t>(*low);
     if ((m & space) == space && __locale::__iswspace(ch, __l_))
       continue;
     if ((m & print) == print && __locale::__iswprint(ch, __l_))

@ldionne ldionne force-pushed the review/locale-iswctype branch from beb54d7 to 7abe9b3 Compare January 8, 2025 22:25
@ldionne ldionne merged commit 81ae668 into llvm:main Jan 9, 2025
62 checks passed
@ldionne ldionne deleted the review/locale-iswctype branch January 9, 2025 13:51
@mysterymath
Copy link
Contributor

This patch broke the build of the Fuchsia toolchain; would you mind reverting or quickly fixing forward?

https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8726213549891343345/overview

[2423/2880](61) Building CXX object libcxx/src/CMakeFiles/cxx_shared.dir/vector.cpp.obj
FAILED: libcxx/src/CMakeFiles/cxx_shared.dir/vector.cpp.obj 
/b/s/w/ir/x/w/llvm_build/./bin/clang++ --target=x86_64-unknown-fuchsia --sysroot=/b/s/w/ir/x/w/sdk/arch/x64/sysroot -DLIBCXX_BUILDING_LIBCXXABI -DLIBC_NAMESPACE=__llvm_libc_common_utils -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src -I/b/s/w/ir/x/w/llvm_build/include/x86_64-unknown-fuchsia/c++/v1 -I/b/s/w/ir/x/w/llvm_build/include/c++/v1 -I/b/s/w/ir/x/w/llvm-llvm-project/libcxxabi/include -I/b/s/w/ir/x/w/llvm-llvm-project/runtimes/cmake/Modules/../../../libc --target=x86_64-unknown-fuchsia -I/b/s/w/ir/x/w/sdk/pkg/sync/include -I/b/s/w/ir/x/w/sdk/pkg/fdio/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/x/w/llvm_build/runtimes/runtimes-x86_64-unknown-fuchsia-bins=../../../llvm-llvm-project -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O2 -g -DNDEBUG -std=c++2b -fPIC -UNDEBUG -faligned-allocation -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -fsized-deallocation -Wall -Wextra -Wnewline-eof -Wshadow -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wunused-template -Wformat-nonliteral -Wzero-length-array -Wdeprecated-redundant-constexpr-static-def -Wno-user-defined-literals -Wno-covered-switch-default -Wno-suggest-override -Wno-error -MD -MT libcxx/src/CMakeFiles/cxx_shared.dir/vector.cpp.obj -MF libcxx/src/CMakeFiles/cxx_shared.dir/vector.cpp.obj.d -o libcxx/src/CMakeFiles/cxx_shared.dir/vector.cpp.obj -c /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/vector.cpp
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/vector.cpp:9:
In file included from /b/s/w/ir/x/w/llvm_build/include/c++/v1/vector:326:
In file included from /b/s/w/ir/x/w/llvm_build/include/c++/v1/__vector/vector_bool_formatter.h:15:
In file included from /b/s/w/ir/x/w/llvm_build/include/c++/v1/__format/formatter_bool.h:19:
In file included from /b/s/w/ir/x/w/llvm_build/include/c++/v1/__format/formatter_integral.h:36:
In file included from /b/s/w/ir/x/w/llvm_build/include/c++/v1/__locale:14:
/b/s/w/ir/x/w/llvm_build/include/c++/v1/__locale_dir/locale_base_api.h:197:10: error: use of undeclared identifier 'iswctype_l'
  197 |   return iswctype_l(__ch, __type, __loc);
      |          ^
1 error generated.

@ldionne
Copy link
Member Author

ldionne commented Jan 9, 2025

@mysterymath How does Fuchsia define iswspace_l & friends? I suspect you'd simply need to add a similar definition for iswspace_l.

@petrhosek We've talked about pre-commit CI for Fuchsia more times than I can remember and for years (literally). The way Fuchsia currently interacts with libc++ for these issues is very disruptive, since errors like this are brought up at random moments and after they've been landed. To be clear, I know there's no ill intent from anyone here and it's just a result of the lack of pre-commit CI, but it's still disruptive.

Fuchsia is the only platform that (for some reason) we've tolerated this for. We made a hard request for pre-commit CI to all other officially supported platforms, which they've done. Please consider this an official "hard request" to provide pre-commit CI. I can work with you on setting it up, and once that's done we'll be happy to list Fuchsia as supported on https://libcxx.llvm.org and address issues before they hit main. However, if this doesn't get addressed, we'll start serving the "oh well Fuchsia is not supported" argument in order to protect our productivity. Like I said, I don't think setting this up is even difficult and I'm willing to spend time with you to get it up and running. But I can't do it alone, so what I'm trying to do here in this comment is give you an incentive to prioritize that work.

@mysterymath
Copy link
Contributor

How does Fuchsia define iswspace_l & friends? I suspect you'd simply need to add a similar definition for iswspace_l.

We get ours from __posix_l_fallback.h; I think this patch would need to add a fallback there for iswctype_l.

@nico
Copy link
Contributor

nico commented Jan 10, 2025

(The Fuchsia problem also blocks updating libc++ in Chromium.)

@ldionne
Copy link
Member Author

ldionne commented Jan 10, 2025

(The Fuchsia problem also blocks updating libc++ in Chromium.)

What issue are you running into? I'd expect the usual Linux path doesn't suffer from the problem reported by Fuchsia or we would have seen it in our CI?

@ldionne
Copy link
Member Author

ldionne commented Jan 10, 2025

How does Fuchsia define iswspace_l & friends? I suspect you'd simply need to add a similar definition for iswspace_l.

We get ours from __posix_l_fallback.h; I think this patch would need to add a fallback there for iswctype_l.

#122484

Does that fix your build? Edit: See also #122489 for the long-term way we want to define the locale base API for Fuchsia.

@nico
Copy link
Contributor

nico commented Jan 13, 2025

(The Fuchsia problem also blocks updating libc++ in Chromium.)

What issue are you running into? I'd expect the usual Linux path doesn't suffer from the problem reported by Fuchsia or we would have seen it in our CI?

We build Chromium for Fuchsia.

https://ci.chromium.org/ui/p/chromium/builders/try/fuchsia-x64-cast-receiver-rel/892111/overview

[3755/97772] CXX obj/base/third_party/double_conversion/double_conversion/string-to-double.o
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/base/third_party/double_conversion/double_conversion/string-to-double.o....(too long)
In file included from ../../base/third_party/double_conversion/double-conversion/string-to-double.cc:29:
In file included from ../../third_party/libc++/src/include/locale:208:
In file included from ../../third_party/libc++/src/include/__locale:14:
../../third_party/libc++/src/include/__locale_dir/locale_base_api.h:197:10: error: use of undeclared identifier 'iswctype_l'
  197 |   return iswctype_l(__ch, __type, __loc);
      |          ^
1 error generated.

I'll try restarting the autoroller now that #122484 is in.

@nico
Copy link
Contributor

nico commented Jan 13, 2025

I'll try restarting the autoroller now that #122484 is in.

Looks like our Fuchsia bots are happy again, thanks!

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.

5 participants