Skip to content

[libc++] Guard <codecvt> contents on _LIBCPP_HAS_LOCALIZATION #129112

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
Feb 28, 2025

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Feb 27, 2025

The codecvt class is defined in and the contents of the header don't work when localization is disabled. Without this guard, builds with localization disabled that happen to include could be broken because they would try to include <__locale>, which ends up trying to include the locale base API and eventually platform headers like <xlocale.h> that may not exist.

@ldionne ldionne added this to the LLVM 20.X Release milestone Feb 27, 2025
@ldionne ldionne requested a review from a team as a code owner February 27, 2025 20:24
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Feb 27, 2025
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 27, 2025

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

The codecvt class is defined in <locale> and the contents of the <codecvt> header don't work when localization is disabled. Without this guard, builds with localization disabled that happen to include <codecvt> could be broken because they would try to include <__locale>, which ends up trying to include the locale base API and eventually platform headers like <xlocale.h> that may not exist.


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

1 Files Affected:

  • (modified) libcxx/include/codecvt (+18-13)
diff --git a/libcxx/include/codecvt b/libcxx/include/codecvt
index f7ae804c6789c..0526b8512175f 100644
--- a/libcxx/include/codecvt
+++ b/libcxx/include/codecvt
@@ -58,14 +58,17 @@ class codecvt_utf8_utf16
 #  include <__cxx03/codecvt>
 #else
 #  include <__config>
-#  include <__locale>
-#  include <version>
 
-#  if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-#    pragma GCC system_header
-#  endif
+#  if _LIBCPP_HAS_LOCALIZATION
+
+#    include <__locale>
+#    include <version>
 
-#  if _LIBCPP_STD_VER < 26 || defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT)
+#    if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#      pragma GCC system_header
+#    endif
+
+#    if _LIBCPP_STD_VER < 26 || defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT)
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
@@ -76,7 +79,7 @@ enum _LIBCPP_DEPRECATED_IN_CXX17 codecvt_mode { consume_header = 4, generate_hea
 template <class _Elem>
 class __codecvt_utf8;
 
-#    if _LIBCPP_HAS_WIDE_CHARACTERS
+#      if _LIBCPP_HAS_WIDE_CHARACTERS
 template <>
 class _LIBCPP_EXPORTED_FROM_ABI __codecvt_utf8<wchar_t> : public codecvt<wchar_t, char, mbstate_t> {
   unsigned long __maxcode_;
@@ -115,7 +118,7 @@ protected:
   int do_length(state_type&, const extern_type* __frm, const extern_type* __end, size_t __mx) const override;
   int do_max_length() const _NOEXCEPT override;
 };
-#    endif // _LIBCPP_HAS_WIDE_CHARACTERS
+#      endif // _LIBCPP_HAS_WIDE_CHARACTERS
 
 _LIBCPP_SUPPRESS_DEPRECATED_PUSH
 template <>
@@ -206,7 +209,7 @@ _LIBCPP_SUPPRESS_DEPRECATED_POP
 template <class _Elem, bool _LittleEndian>
 class __codecvt_utf16;
 
-#    if _LIBCPP_HAS_WIDE_CHARACTERS
+#      if _LIBCPP_HAS_WIDE_CHARACTERS
 template <>
 class _LIBCPP_EXPORTED_FROM_ABI __codecvt_utf16<wchar_t, false> : public codecvt<wchar_t, char, mbstate_t> {
   unsigned long __maxcode_;
@@ -284,7 +287,7 @@ protected:
   int do_length(state_type&, const extern_type* __frm, const extern_type* __end, size_t __mx) const override;
   int do_max_length() const _NOEXCEPT override;
 };
-#    endif // _LIBCPP_HAS_WIDE_CHARACTERS
+#      endif // _LIBCPP_HAS_WIDE_CHARACTERS
 
 _LIBCPP_SUPPRESS_DEPRECATED_PUSH
 template <>
@@ -451,7 +454,7 @@ _LIBCPP_SUPPRESS_DEPRECATED_POP
 template <class _Elem>
 class __codecvt_utf8_utf16;
 
-#    if _LIBCPP_HAS_WIDE_CHARACTERS
+#      if _LIBCPP_HAS_WIDE_CHARACTERS
 template <>
 class _LIBCPP_EXPORTED_FROM_ABI __codecvt_utf8_utf16<wchar_t> : public codecvt<wchar_t, char, mbstate_t> {
   unsigned long __maxcode_;
@@ -490,7 +493,7 @@ protected:
   int do_length(state_type&, const extern_type* __frm, const extern_type* __end, size_t __mx) const override;
   int do_max_length() const _NOEXCEPT override;
 };
-#    endif // _LIBCPP_HAS_WIDE_CHARACTERS
+#      endif // _LIBCPP_HAS_WIDE_CHARACTERS
 
 _LIBCPP_SUPPRESS_DEPRECATED_PUSH
 template <>
@@ -579,7 +582,9 @@ _LIBCPP_SUPPRESS_DEPRECATED_POP
 
 _LIBCPP_END_NAMESPACE_STD
 
-#  endif // _LIBCPP_STD_VER < 26 || defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT)
+#    endif // _LIBCPP_STD_VER < 26 || defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCPP_ENABLE_CXX26_REMOVED_CODECVT)
+
+#  endif // _LIBCPP_HAS_LOCALIZATION
 
 #  if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
 #    include <atomic>

@github-project-automation github-project-automation bot moved this from Needs Triage to Needs Merge in LLVM Release Status Feb 27, 2025
Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

I feel like we need a better story for this kind of exclusion. It's getting quite viral. But I guess it's fine for now.

@ldionne
Copy link
Member Author

ldionne commented Feb 28, 2025

I feel like we need a better story for this kind of exclusion. It's getting quite viral. But I guess it's fine for now.

I agree. I am working on actually removing most of the locale-based exclusion. By formalizing the locale base API, my hope is that the "no localization" setting can instead be a "emulate only the C locale" setting, which would allow us to provide regex, iostreams and more in the "no localization" setup, but without being able to actually switch to another locale than C.

The codecvt class is defined in <locale> and the contents of the <codecvt>
header don't work when localization is disabled. Without this guard, builds
with localization disabled that happen to include <codecvt> could be broken
because they would try to include <__locale>, which ends up trying to include
the locale base API and eventually platform headers like <xlocale.h> that may
not exist.
@ldionne ldionne force-pushed the review/fix-localization-codecvt branch from 38afaa0 to dd78129 Compare February 28, 2025 15:22
@ldionne
Copy link
Member Author

ldionne commented Feb 28, 2025

@philnik777 I think your recent patch about std::async has issues on FreeBSD -- likely another race condition with the re-enablement of the CI job. Can you have a look? I'm about to go OOO for a bit and I won't have time to look into that one.

@ldionne
Copy link
Member Author

ldionne commented Feb 28, 2025

Either way, that failure isn't related to this change so I'm going ahead with merging this.

@ldionne ldionne merged commit fda7373 into llvm:main Feb 28, 2025
79 of 83 checks passed
@ldionne ldionne deleted the review/fix-localization-codecvt branch February 28, 2025 20:41
@github-project-automation github-project-automation bot moved this from Needs Merge to Done in LLVM Release Status Feb 28, 2025
@ldionne
Copy link
Member Author

ldionne commented Feb 28, 2025

/cherry-pick fda7373

@llvmbot
Copy link
Member

llvmbot commented Feb 28, 2025

/pull-request #129305

swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Mar 11, 2025
…29112)

The codecvt class is defined in <locale> and the contents of the
<codecvt> header don't work when localization is disabled. Without this
guard, builds with localization disabled that happen to include
<codecvt> could be broken because they would try to include <__locale>,
which ends up trying to include the locale base API and eventually
platform headers like <xlocale.h> that may not exist.

(cherry picked from commit fda7373)
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
…29112)

The codecvt class is defined in <locale> and the contents of the
<codecvt> header don't work when localization is disabled. Without this
guard, builds with localization disabled that happen to include
<codecvt> could be broken because they would try to include <__locale>,
which ends up trying to include the locale base API and eventually
platform headers like <xlocale.h> that may not exist.
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
Development

Successfully merging this pull request may close these issues.

4 participants