[libc++] Reduce the dependency of the locale base API on the base system from the headers #117764
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesMany parts of the locale base API are only required when building the shared/static library, but not from the headers. Document those functions and carve out a few of those that don't work when _XOPEN_SOURCE is defined to something old. Fixes #117630 Full diff: https://github.com/llvm/llvm-project/pull/117764.diff 3 Files Affected:
diff --git a/libcxx/include/__locale_dir/locale_base_api.h b/libcxx/include/__locale_dir/locale_base_api.h
index 8ed4c29cb8732f..89bb2a724fff09 100644
--- a/libcxx/include/__locale_dir/locale_base_api.h
+++ b/libcxx/include/__locale_dir/locale_base_api.h
@@ -23,13 +23,17 @@
// Variadic functions may be implemented as templates with a parameter pack instead
// of C-style variadic functions.
//
+// Most of these functions are only required when building the library. Functions that are also
+// required when merely using the headers are marked as such below.
+//
// TODO: I think __uselocale() is not necessary if we refactor a bit.
// TODO: __localeconv shouldn't take a reference, but the Windows implementation doesn't allow copying __locale_t
+// TODO: Eliminate the need for any of these functions from the headers.
//
// Locale management
// -----------------
// namespace __locale {
-// using __locale_t = implementation-defined;
+// using __locale_t = implementation-defined; // required by the headers
// __locale_t __uselocale(__locale_t);
// __locale_t __newlocale(int, const char*, __locale_t);
// void __freelocale(__locale_t);
@@ -51,8 +55,8 @@
// namespace __locale {
// int __islower(int, __locale_t);
// int __isupper(int, __locale_t);
-// int __isdigit(int, __locale_t);
-// int __isxdigit(int, __locale_t);
+// int __isdigit(int, __locale_t); // required by the headers
+// int __isxdigit(int, __locale_t); // required by the headers
// int __toupper(int, __locale_t);
// int __tolower(int, __locale_t);
// int __strcoll(const char*, const char*, __locale_t);
@@ -89,9 +93,10 @@
// int __mbtowc(wchar_t*, const char*, size_t, __locale_t);
// size_t __mbrlen(const char*, size_t, mbstate_t*, __locale_t);
// size_t __mbsrtowcs(wchar_t*, const char**, size_t, mbstate_t*, __locale_t);
-// int __snprintf(char*, size_t, __locale_t, const char*, ...);
-// int __asprintf(char**, __locale_t, const char*, ...);
-// int __sscanf(const char*, __locale_t, const char*, ...);
+//
+// int __snprintf(char*, size_t, __locale_t, const char*, ...); // required by the headers
+// int __asprintf(char**, __locale_t, const char*, ...); // required by the headers
+// int __sscanf(const char*, __locale_t, const char*, ...); // required by the headers
// }
#if defined(__APPLE__)
diff --git a/libcxx/include/__locale_dir/support/bsd_like.h b/libcxx/include/__locale_dir/support/bsd_like.h
index cce6de64673b0e..ef9bf8c07543b2 100644
--- a/libcxx/include/__locale_dir/support/bsd_like.h
+++ b/libcxx/include/__locale_dir/support/bsd_like.h
@@ -144,19 +144,23 @@ inline _LIBCPP_HIDE_FROM_ABI wint_t __btowc(int __c, __locale_t __loc) { return
inline _LIBCPP_HIDE_FROM_ABI int __wctob(wint_t __c, __locale_t __loc) { return ::wctob_l(__c, __loc); }
+#ifdef _LIBCPP_BUILDING_LIBRARY
inline _LIBCPP_HIDE_FROM_ABI size_t
__wcsnrtombs(char* __dest, const wchar_t** __src, size_t __nwc, size_t __len, mbstate_t* __ps, __locale_t __loc) {
return ::wcsnrtombs_l(__dest, __src, __nwc, __len, __ps, __loc);
}
+#endif
inline _LIBCPP_HIDE_FROM_ABI size_t __wcrtomb(char* __s, wchar_t __wc, mbstate_t* __ps, __locale_t __loc) {
return ::wcrtomb_l(__s, __wc, __ps, __loc);
}
+#ifdef _LIBCPP_BUILDING_LIBRARY
inline _LIBCPP_HIDE_FROM_ABI size_t
__mbsnrtowcs(wchar_t* __dest, const char** __src, size_t __nms, size_t __len, mbstate_t* __ps, __locale_t __loc) {
return ::mbsnrtowcs_l(__dest, __src, __nms, __len, __ps, __loc);
}
+#endif
inline _LIBCPP_HIDE_FROM_ABI size_t
__mbrtowc(wchar_t* __pwc, const char* __s, size_t __n, mbstate_t* __ps, __locale_t __loc) {
diff --git a/libcxx/test/libcxx/xopen_source.gen.py b/libcxx/test/libcxx/xopen_source.gen.py
new file mode 100644
index 00000000000000..94298de0289fd3
--- /dev/null
+++ b/libcxx/test/libcxx/xopen_source.gen.py
@@ -0,0 +1,41 @@
+# ===----------------------------------------------------------------------===##
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+# ===----------------------------------------------------------------------===##
+
+# Make sure that libc++ headers work when defining _XOPEN_SOURCE=500.
+# We may not want to guarantee this forever, but since this works today and
+# it's something that users rely on, it makes sense to put a test on it.
+#
+# https://github.com/llvm/llvm-project/issues/117630
+
+# RUN: %{python} %s %{libcxx-dir}/utils
+
+import sys
+
+sys.path.append(sys.argv[1])
+from libcxx.header_information import (
+ lit_header_restrictions,
+ lit_header_undeprecations,
+ public_headers,
+)
+
+for header in public_headers:
+ for version in (500, 600, 700):
+ # TODO: <fstream> currently uses ::fseeko unguarded, which fails with _XOPEN_SOURCE=500.
+ if header == 'fstream' and version == 500:
+ continue
+
+ print(f"""\
+//--- {header}.xopen_source_{version}.compile.pass.cpp
+{lit_header_restrictions.get(header, '')}
+{lit_header_undeprecations.get(header, '')}
+
+// ADDITIONAL_COMPILE_FLAGS: -D_XOPEN_SOURCE={version}
+
+#include <{header}>
+"""
+ )
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@nico Looking at the CI failures (on AIX, on FreeBSD and on macOS), it looks like much of the library was already broken when defining I didn't think this was going to be my conclusion, but I am tempted to conclude that such legacy values for this macro might not be worth supporting. Doing it properly certainly seems to require a non-trivial amount of refactoring. Do you have thoughts? |
|
As I said on the issue, it looks pretty common: https://grep.app/search?q=_XOPEN_SOURCE%20500&filter[lang][0]=C%2B%2B I don't think calling the value "legacy" is terribly useful. People set it to some value because they want to use a function that was added at that value. I'm guessing they probably set it to either the value that is highest when they set it, or to the lowest value needed for whatever function they want – it's more a "set it to this value at least please". OTOH, it's probably also possible that some folks out there want to set it as a "set it to this at most please" if they want to deploy to older systems, I don't know. The thing we hit in practice is that Since it was easy to work around in Chromium, I don't have a super strong opinion on this particular issue. (I do have the opinion that libc++ generally tends to be not very friendly to update. We're currently 3 weeks behind again, just down from 5 weeks, due to at least 4 things that need some kind of tweak landing in fast succession. And this year, our libc++ autoroller needed significantly more handholding than last year. This isn't really the forum for that, though :) If there's interest, I can do an experience report writeup for this year – but I'm guessing this is all working as intended.) |
You are an early adopter, so that's not exactly surprising. It would be interesting to know where the hiccups are though. |
Actually, almost all of the hits in a file named
The problem is that I don't know how to fix it "properly". I can certainly silence this specific error, but as seen in the the CI failures, "alternate" (to avoid the world "legacy") definitions of I guess we could do that as a stopgap until we refactor things such that we truly don't care about what that macro is defined to in the future, if that ever happens. I think that would require defining some symbols in the dylib (where we control the build flags).
I'd be curious to know what roadblocks you encountered in your recent rolls. While you're an early adopter and updates can't be 0 work, we've been trying to provide transition paths for most potentially-breaking changes we make (partly as a result of past feedback like yours). I'd like to know in what cases that didn't work for you so we can either say "yeah, that one was intended" or improve in the future. |
ldionne
force-pushed
the
review/fix-xopen-source
branch
from
1d1e32b to
a486593
Compare
|
It turns out this is breaking more code than I thought. I'll try to push this through the finish line and cherry-pick into LLVM 20. |
ldionne
force-pushed
the
review/fix-xopen-source
branch
from
a486593 to
f7e6a7a
Compare
…tem from the headers Many parts of the locale base API are only required when building the shared/static library, but not from the headers. Document those functions and carve out a few of those that don't work when _XOPEN_SOURCE is defined to something old. Fixes llvm#117630
ldionne
force-pushed
the
review/fix-xopen-source
branch
from
a93cdc3 to
1c0610b
Compare
|
/cherry-pick f00b32e |
|
Failed to cherry-pick: f00b32e https://github.com/llvm/llvm-project/actions/runs/13436600368 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
|
/pull-request #128009 |
…tem from the headers (llvm#117764) Many parts of the locale base API are only required when building the shared/static library, but not from the headers. Document those functions and carve out a few of those that don't work when _XOPEN_SOURCE is defined to something old. Fixes llvm#117630 (cherry picked from commit f00b32e)
Many parts of the locale base API are only required when building the shared/static library, but not from the headers. Document those functions and carve out a few of those that don't work when _XOPEN_SOURCE is defined to something old.
Fixes #117630