[libc++] Avoid calling setlocale in do_unshift when unnecessary

[m417z]

This is an attempt to mitigate #110954.

As part of the libc++.dll initialization, static DoIOSInit is initialized:

llvm-project/libcxx/src/iostream.cpp

Lines 165 to 167 in 5bdee35

	ios_base::Init::Init() {
	static DoIOSInit init_the_streams; // gets initialized once
	}

When the dll is unloaded or on process shutdown, DoIOSInit::~DoIOSInit is called. It ends up calling flush:

llvm-project/libcxx/src/iostream.cpp

Line 159 in 5bdee35

wcout_ptr->flush();

Which calls pubsync:

llvm-project/libcxx/include/__ostream/basic_ostream.h

Line 659 in 5bdee35

if (this->rdbuf()->pubsync() == -1)

Which ends up calling do_unshift:

llvm-project/libcxx/src/locale.cpp

Lines 1475 to 1479 in 5bdee35

	codecvt<wchar_t, char, mbstate_t>::result codecvt<wchar_t, char, mbstate_t>::do_unshift(
	state_type& st, extern_type* to, extern_type* to_end, extern_type*& to_nxt) const {
	to_nxt = to;
	extern_type tmp[MB_LEN_MAX];
	size_t n = __locale::__wcrtomb(tmp, intern_type(), &st, __l_);

Which, as can be seen, unconditionally calls __locale::__wcrtomb, which ends up calling setlocale via __libcpp_locale_guard.

All this means that setlocale is called on process shutdown even if wcout is never used, or even if nothing stream-related is used. Calling setlocale on process shutdown causes problems, as described in the mentioned issue.

This PR is an attempt to avoid calling setlocale in the vast majority of cases, when there's no output to be flushed. It's not a complete fix to the issue, but it will make it much less common, and it will at least allow to flush output manually to avoid the issue if streams are used.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Michael Maltsev (m417z)

Changes

This is an attempt to mitigate #110954.

As part of the libc++.dll initialization, static DoIOSInit is initialized:

llvm-project/libcxx/src/iostream.cpp

Lines 165 to 167 in 5bdee35

	ios_base::Init::Init() {
	static DoIOSInit init_the_streams; // gets initialized once
	}

When the dll is unloaded or on process shutdown, DoIOSInit::~DoIOSInit is called. It ends up calling flush:

llvm-project/libcxx/src/iostream.cpp

Line 159 in 5bdee35

wcout_ptr->flush();

Which calls pubsync:

llvm-project/libcxx/include/__ostream/basic_ostream.h

Line 659 in 5bdee35

if (this->rdbuf()->pubsync() == -1)

Which ends up calling do_unshift:

llvm-project/libcxx/src/locale.cpp

Lines 1475 to 1479 in 5bdee35

	codecvt<wchar_t, char, mbstate_t>::result codecvt<wchar_t, char, mbstate_t>::do_unshift(
	state_type& st, extern_type* to, extern_type* to_end, extern_type*& to_nxt) const {
	to_nxt = to;
	extern_type tmp[MB_LEN_MAX];
	size_t n = __locale::__wcrtomb(tmp, intern_type(), &st, __l_);

Which, as can be seen, unconditionally calls __locale::__wcrtomb, which ends up calling setlocale via __libcpp_locale_guard.

All this means that setlocale is called on process shutdown even if wcout is never used, or even if nothing stream-related is used. Calling setlocale on process shutdown causes problems, as described in the mentioned issue.

This PR is an attempt to avoid calling setlocale in the vast majority of cases, when there's no output to be flushed. It's not a complete fix to the issue, but it will make it much less common, and it will at least allow to flush output manually to avoid the issue if streams are used.

---

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

1 Files Affected:

• (modified) libcxx/src/locale.cpp (+2)

diff --git a/libcxx/src/locale.cpp b/libcxx/src/locale.cpp
index a1e10401f0b299..5ecd99c53cd516 100644
--- a/libcxx/src/locale.cpp
+++ b/libcxx/src/locale.cpp
@@ -1475,6 +1475,8 @@ codecvt<wchar_t, char, mbstate_t>::result codecvt<wchar_t, char, mbstate_t>::do_
 codecvt<wchar_t, char, mbstate_t>::result codecvt<wchar_t, char, mbstate_t>::do_unshift(
     state_type& st, extern_type* to, extern_type* to_end, extern_type*& to_nxt) const {
   to_nxt = to;
+  if (std::mbsinit(&st))
+    return ok;
   extern_type tmp[MB_LEN_MAX];
   size_t n = __locale::__wcrtomb(tmp, intern_type(), &st, __l_);
   if (n == size_t(-1) || n == 0) // on error

[cpplearner]

I wonder if we should remove the unshift call in sync().

llvm-project/libcxx/include/fstream

Lines 965 to 983 in c4d656a

	template <class _CharT, class _Traits>
	int basic_filebuf<_CharT, _Traits>::sync() {
	if (__file_ == nullptr)
	return 0;
	if (!__cv_)
	__throw_bad_cast();
	if (__cm_ & ios_base::out) {
	if (this->pptr() != this->pbase())
	if (overflow() == traits_type::eof())
	return -1;
	codecvt_base::result __r;
	do {
	char* __extbe;
	__r = __cv_->unshift(__st_, __extbuf_, __extbuf_ + __ebs_, __extbe);
	size_t __nmemb = static_cast<size_t>(__extbe - __extbuf_);
	if (fwrite(__extbuf_, 1, __nmemb, __file_) != __nmemb)
	return -1;
	} while (__r == codecvt_base::partial);

The standard doesn't require such a call. [filebuf.virtuals]/19:

int sync() override;

Effects: If a put area exists, calls filebuf​::​overflow to write the characters to the file, then flushes the file as if by calling fflush(file). If a get area exists, the effect is implementation-defined.

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

[ldionne]

I'm not the best person to review this PR because I'm not very well-versed in locales and all this multibyte character fun. But this seems correct to me based on cppreference's interpretation of the Standard, which is better than nothing:

If the encoding represented by this codecvt facet is state-dependent, and state represents a conversion state that is not the initial shift state, writes the characters necessary to return to the initial shift state.

The effects in the actual Standard don't mention this specifically but I assume this is obvious to anyone more familiar with this API.

It would be nice to get a short review from someone like @tahonermann or @mstorsjo.

Assuming this is functionally correct, I have no objection about short-circuiting in this function to minimize the impact of the linked bug.

[mstorsjo]

Hopefully @tahonermann has time to look at this; I'm unfortunately not very familiar with this area. In particular, I don't quite follow how the PR description (which sounds very reasonable) maps to this piece of code - how mbsinit(mbstate_t*) checks whether there are buffered characters or not. (And wouldn't we want to do that check only in some destructor, rather than adding a check every time we do do_unshift?)

[m417z]

@mstorsjo thanks for taking a look. This fix is mostly an educated guess based on my best understanding from skimming over the code. I hoped that this simple two-liner fix will either just work, or will get a trivial correction from somebody who's familiar with this area.

how mbsinit(mbstate_t*) checks whether there are buffered characters or not

Maybe I'm completely off here (I wasn't familiar with this function prior to looking at this area), but from the description and the code example of this function, my best understanding is that mbstate_t acts as a kind of storage for partial character representation. So if it's in the initial state, there's nothing to be done, so we can just return early.

And wouldn't we want to do that check only in some destructor, rather than adding a check every time we do do_unshift?

I figured that it doesn't really matter. If anything, it acts as an optimization for the common case, which might be significant or not depending on the implementation of __locale::__wcrtomb.

Here, it was mentioned that a more complete fix would be to have some of the missing Windows string functions implemented:

It seems to me like the fundamental problem is that windows is missing quite a few _l versions. Is there any way we could work around that problem instead to avoid the __locale_guards altogether? CC @mstorsjo

Did you see this comment? Any thoughts about it?

[mstorsjo]

@mstorsjo thanks for taking a look. This fix is mostly an educated guess based on my best understanding from skimming over the code. I hoped that this simple two-liner fix will either just work, or will get a trivial correction from somebody who's familiar with this area.

how mbsinit(mbstate_t*) checks whether there are buffered characters or not

Maybe I'm completely off here (I wasn't familiar with this function prior to looking at this area), but from the description and the code example of this function, my best understanding is that mbstate_t acts as a kind of storage for partial character representation. So if it's in the initial state, there's nothing to be done, so we can just return early.

Ok, so this is only about flushing incomplete multibyte chars? Then I guess this may seem reasonable.

Here, it was mentioned that a more complete fix would be to have some of the missing Windows string functions implemented:

It seems to me like the fundamental problem is that windows is missing quite a few _l versions. Is there any way we could work around that problem instead to avoid the __locale_guards altogether? CC @mstorsjo

Did you see this comment? Any thoughts about it?

Ah, right - I never got around to commenting on it. Offhand I don't know of any good way to avoid that problem, short of manually implementing them. I believe that would require access to the internals of the existing functions in order to share code/logic with them, though, or then just write complete reimplementations from scratch. But I haven't spent any effort on researching this area.

[m417z]

Offhand I don't know of any good way to avoid that problem, short of manually implementing them

My thought was that replacing setlocale + wcrtomb with wcrtomb_l will work, but looks like MS doesn't offer wcrtomb_l, although it does offer other _l functions such as _wctomb_l. Given that, perhaps the only solution in this case is indeed having a manual implementation, which is not ideal.