[libcxx] Fix using std::wcout/wcin on Windows with streams configured in wide mode

[mstorsjo]

Summary

On Windows, the underlying file descriptors for stdout/stdin/stderr
can be reconfigured to wide mode. In the default (narrow) mode, the
charset usually isn't utf8 (as libcxx assumes), but normally a locale
specific codepage (where each codepage only can represent a small
subset of unicode characters).

By configuring the stdout file descriptor to wide mode, the user can
output wchar_t based strings without convesion to the narrow charset.
Within libcxx, don't try to use codecvt to convert this to a narrow
character encoding, but output these strings as such with fputwc.

In wide mode, such strings could be output directly with fwrite too,
but if the file descriptor hasn't been configured in wide mode, that
breaks the output (which currently works reasonably). By always
outputting one character at a time with fputwc, it works regardless
of mode of the stdout file descriptor.

For the narrow output stream, std::cout, outputting (via fwrite)
does fail when the file descriptor is set to wide mode. This matches
how it behaves with both MS STL and GNU libstdc++ too, so this is
probably acceptable.

This fixes https://github.com/llvm/llvm-project/issues/46646, and
the downstream bugs https://github.com/mstorsjo/llvm-mingw/issues/145
and https://github.com/mstorsjo/llvm-mingw/issues/222.

The patch itself isn't very pretty, I would have preferred to get
by with much much fewer ifdefs. Initially, I considered adding
another template type for the char type to use for the stream
(which would be char on other platforms, and equal to char_type on
Windows), but codecvt doesn't support the other type being anything
other than char.

[mstorsjo]

Rebased on latest git, to get a cleaner CI run.

[mstorsjo]

@fsb4000 Can you have a look at this, does it seem reasonable to you? (As mentioned in the patch description, I'm not a fan of the amount of ifdefs that this adds, but I didn't see a good way of getting away with fewer of them either.

[fsb4000]

Sorry, I didn't have time on the weekdays. But I have time today.

Yes, it looks good.

I restarted CI but it seems it doesn't work currently...

And I checked on my machine with _O_WTEXT and _O_U16TEXT that issue example in a debugger.

#include <iostream>
#include <io.h>
#include <fcntl.h>

int main()
{
	_setmode(_fileno(stdout), _O_U16TEXT);
	std::wcout << L"Thử nghiệmПривет\n";
	return 0;
}

By the way how should I apply the patch locally?

I tried this way:

1. Save file https://reviews.llvm.org/file/data/4ebdk4eqonefejv45365/PHID-FILE-kpusoqwct45nr6shhmdk/D146398.diff

2. git checkout f721fcb

3. git apply D146398.diff

But I got an error that git couldn't apply. So I just manually copy-pasted the changes locally.

[mstorsjo]

In D146398#4221400, @fsb4000 wrote:

Sorry, I didn't have time on the weekdays. But I have time today.

Yes, it looks good.

Thanks for having a look!

I restarted CI but it seems it doesn't work currently...

Yeah... The remaining failures from before were unrelated to this patch anyway.

By the way how should I apply the patch locally?

I tried this way:

1. Save file https://reviews.llvm.org/file/data/4ebdk4eqonefejv45365/PHID-FILE-kpusoqwct45nr6shhmdk/D146398.diff

2. git checkout f721fcb

3. git apply D146398.diff

But I got an error that git couldn't apply. So I just manually copy-pasted the changes locally.

That's weird - I tried those exact steps, and it succeeded for me. If git apply is too fussy, I'd try with patch -p1 < D146398.diff too (after clearing up any potential intermediate state from a previous failed application of the patch).

The easiest way generally is if you have the arc tool available (and patched to work with newer versions of PHP, if you happen to have that) - then you can do just arc patch --nobranch D146398 to have it download and apply the patch in the current branch.

[Mordante]

Is it possible to add tests for these changes?

libcxx/include/__std_stream

87

if constexpr fails before C++17, AFAIK this code should be working in all language modi.

124

Is this ifdef really needed? Will it fail on Linux when using char_type?

[mstorsjo]

In D146398#4253025, @Mordante wrote:

Is it possible to add tests for these changes?

I’d love to, but it’s not very straightforward.. We already lack coverage of the conversion aspects of std::cout/std::wcout - the most complex thing we test is to print 1234 to the output streams and check that it ends up printing the right thing with a bash script. To test the relevant aspects here, we’d need to print unicode charts, and I’m not sure how well that integrates with bash here (or how well llvm-lit manages to pass the unicode string to validate to the script).

libcxx/include/__std_stream

87

Yeah, we can’t use it in public headers, but as far as I could see, this header isn’t ever accessed by any public header - it should only be included by the libcxx sources.

I think it might be fine to leave out the constexpr if you want it out of principle though, I don’t think there was a functional need for it.

124

Yes; the type of __extbuf needs to be the right one for the conversion from the external charset (utf8 in a char array on other systems, but read directly as wchars on windows) into char_type - see the __cv_->in call below on line 138/164.

[philnik]

libcxx/include/__std_stream

87

Could you move the header to src/ if it's actually a private one? (in another patch)

[mstorsjo]

Added testcases - it turned out to be quite straightforward to pipe things to a file and compare with a reference file (which contains raw wchars).

These new testcases are windows specific though, as they use _setmode() to put stdout/stderr/stdin in wide char mode.

I kept the if constexpr for now; this can be pushed after D147855 when the header is made properly private under src.

[mstorsjo]

Rebased onto the move of the __std_stream header into the src directory.

This does now include a test (since the last update), which seems to run correctly in the CI environment.

[tahonermann]

This doesn't seem like the right approach to me.

First, the code changes aren't actually sensitive to the mode of the file handle so the calls to _setmode() in the tests presumably have no behavioral impact (other than they may change the behavior of functions called by the changed code).

Second, but related to the first point above, the code changes are isolated to the __stdinbuf and __stdoutbuf classes which are only used to wrap stdin, stdout, and stderr; thus the changes won't impact other file handles for which _setmode() has been called. I don't think libc++ currently exposes the FILE* pointers held in basic_filebuf instances, but future changes may require doing so. See https://wg21.link/p1759 for example.

It probably is necessary to #ifdef the code to enable Windows-specific behavior. However, I don't think the sizeof() checks are what should be used for conditional behavior. Rather, I think the file handle mode should determine whether, e.g., getwc() or getc() is called. If it really is necessary to conditionalize some of the code for char_type, then I think that should be done with is_same_v<char_type, wchar_t> rather than using sizeof as a proxy for the type.

[mstorsjo]

Hi @tahonermann, thanks for having a look!

In D146398#4266539, @tahonermann wrote:

First, the code changes aren't actually sensitive to the mode of the file handle so the calls to _setmode() in the tests presumably have no behavioral impact (other than they may change the behavior of functions called by the changed code).

The "may change the behaviour of functions called by the changed code" is the core of the issue here. And if the _setmode() call is commented out in the tests, the test output does change (and the test fails).

Actually, as far as I know, there's no way to inspect the current mode set on a file descriptor - so even if we wanted to, we can't. (And other C++ libraries such as MS STL don't inspect the mode either.)

The core of the issue is that previously, when writing wchars, we converted them to a narrow charset (utf8, even if the right thing for Windows would be the current active codepage) and wrote them using fwrite(). Whenever writing with fwrite(), the data gets written as such, but when writing with fputwc(), each individual wchar gets passed through to the CRT without getting mangled, and the CRT converts it to the right output based on the file descriptor's current mode (which we can't observe outside of the CRT).

Thus, after this change, for wchar based stdout/stderr streams, we're always writing with fputwc(), which lets the CRT handle it according to mode. (For streams based on regular chars, we're still writing with fwrite() - which does break output when the streams are changed to wide mode. However both MS STL and GNU libstdc++ seem to fail to output things in that mode as well, so I would consider that combination pathological.)

Second, but related to the first point above, the code changes are isolated to the __stdinbuf and __stdoutbuf classes which are only used to wrap stdin, stdout, and stderr; thus the changes won't impact other file handles for which _setmode() has been called. I don't think libc++ currently exposes the FILE* pointers held in basic_filebuf instances, but future changes may require doing so. See https://wg21.link/p1759 for example.

Right, that's a good point. But for the current implementation in e.g. __stdoutbuf, the writes get done at that level, so whatever extra logic we place in other classes won't get hit if __stdoutbuf does the writing on its own (unless we refactor it all significantly - which I'm not really wanting to take on at the moment).

Secondly, as a detail, that spec draft seems to expose the deepest native handle which seems to be meant to be a HANDLE on Windows - the suggested code for Windows uses _fileno() to get a file descriptor from a FILE*, and then uses _get_osfhandle() to get the underlying HANDLE. The modes set by _setmode() operate on the CRT level file descriptor - the value returned by _fileno() - not the underlying OS level HANDLE. So even with that HANDLE exposed from such a stream, I don't think you can access the CRT level file descriptor to change its mode. (With the Linux definition of native_handle_type, where it is the file descriptor returned by fileno, it would be possible to change the mode on Windows though.)

Thus, while that could be an issue in some future, requiring similar changes in other classes, I'd prefer to focus at the current state of affairs here.

It probably is necessary to #ifdef the code to enable Windows-specific behavior. However, I don't think the sizeof() checks are what should be used for conditional behavior. Rather, I think the file handle mode should determine whether, e.g., getwc() or getc() is called.

Unfortunately - as mentioned above, the file descriptor mode isn't accessible - and I think it would make for some kinda clumsy code to be querying for it for every single write; neither MS STL nor GNU libstdc++ do that.

If it really is necessary to conditionalize some of the code for char_type, then I think that should be done with is_same_v<char_type, wchar_t> rather than using sizeof as a proxy for the type.

Thanks, that would indeed be better; when I initially wrote this I wanted to do it like that, but my C++-fu isn't strong enough so I went with the simplest constant expression I could come up with, and forgot to attach a question about what the more correct conditional would be. I'll amend it to use that and update the patch.

[mstorsjo]

Changed to use is_same_v<char_type, wchar_t> instead of a sizeof check.

Btw, should the added tests be under the libcxx tree, or in place under the std tree? It doesn't test any nonstandard libc++ APIs, but it uses Windows specifics to set up the test environment.

[Mordante]

In D146398#4267374, @mstorsjo wrote:

Changed to use is_same_v<char_type, wchar_t> instead of a sizeof check.

Btw, should the added tests be under the libcxx tree, or in place under the std tree? It doesn't test any nonstandard libc++ APIs, but it uses Windows specifics to set up the test environment.

Typically we still do these under std, but if the setup code uses non-standard behaviour then libcxx would be a better place.
In general all tests under std should work out-of-the box with other Standard libraries.

libcxx/src/std_stream.h

87 ↗

(On Diff #513465)

Since we conditionally support wchar_t I have a preference to check whether the type is not char.

155 ↗

(On Diff #513465)

How bad would it be to generate this dead code? It current code looks a bit tricky.

How do you feel about adding a helper variable

#if defined(_LIBCPP_WIN32API)
template<class CharT>
inline constexpr bool __is_win32api_wide_char = !is_same_v<_CharT, char>;
#else
template<class>
inline constexpr bool __is_win32api_wide_char = false;
#endif

and use that? That also removes the #ifdef. Can you test locally whether that makes the code better readable?

[mstorsjo]

In D146398#4274336, @Mordante wrote:

In D146398#4267374, @mstorsjo wrote:

Changed to use is_same_v<char_type, wchar_t> instead of a sizeof check.

Btw, should the added tests be under the libcxx tree, or in place under the std tree? It doesn't test any nonstandard libc++ APIs, but it uses Windows specifics to set up the test environment.

Typically we still do these under std, but if the setup code uses non-standard behaviour then libcxx would be a better place.
In general all tests under std should work out-of-the box with other Standard libraries.

Thanks; these tests should indeed pass with MS STL and GNU libstdc++, on Windows.

libcxx/src/std_stream.h

87 ↗

(On Diff #513465)

Sure, I can change that.

155 ↗

(On Diff #513465)

Yes, this is very much not great, but unfortunately we need to avoid generating this dead code here, unless we do much larger refactorings here. Maybe there's a better way to do it though?

We need to change the type of __extbuf (line 125) in order to not truncate the chars.

If we try to generate dead code for this block here, the __cv_->in call below fails since __extbuf has got the wrong type for the conversion source for the codecvt. __cv_ is defined as const codecvt<char_type, char, state_type>* __cv_; Ideally, I'd prefer to make it const codecvt<char_type, char_type, state_type>* __cv_; for Windows, i.e. making it a no-op (which we wouldn't call anyway), but codecvt doesn't support that, it only supports combinations where the second type is char - it can't be <wchar_t, wchar_t>.

So by setting __always_noconv_ we avoid trying to use the codecvt at runtime, but we need to skip actual code generation for this case here...

Thanks for the tip with __is_win32api_wide_char, let me try that! That could simplify this (and other cases) a fair bit.

[mstorsjo]

libcxx/src/std_stream.h

155 ↗

(On Diff #513465)

Thanks, that suggestion with __is_win32api_wide_char made the change much more palatable!

[mstorsjo]

Split out the main condition to a constexpr bool variable, as suggested by @Mordante, singificantly reducing the amount of ifdefs.

[Mordante]

Thanks for the update, I like the current version a lot more. Some minor comments, but in general I'm happy.

libcxx/src/std_stream.h

66 ↗

(On Diff #514393)

I actually intended it not to be in the class. I don't mind it, but then we don't need the inline.

155 ↗

(On Diff #513465)

If this indeed avoids generating the dead code I prefer this solution.

libcxx/test/std/input.output/iostream.objects/wide.stream.objects/check-stderr.sh

1 ↗

(On Diff #514393)

Can you add some comments to these test explaining what they do.

[mstorsjo]

libcxx/src/std_stream.h

66 ↗

(On Diff #514393)

Yeah, it looked like that, but I wasn’t sure of how that’d work with the template parameter CharT - wouldn’t I need to pass it on from each of the class templates to the template bool variable then? (This is a bit outside of the C++ template syntax I’m proficient about…)

libcxx/test/std/input.output/iostream.objects/wide.stream.objects/check-stderr.sh

1 ↗

(On Diff #514393)

Sure, can do. (These are copies of similar scripts in one directory above, but these take the reference from a file - which in this case is written as UCS2/UTF16 - while the existing test scripts take the reference string as a command line parameter.)

[Mordante]

libcxx/src/std_stream.h

66 ↗

(On Diff #514393)

Yes then you needed to pass them in if constexpr. But this in fine too (modulo the inline).

[mstorsjo]

Added comments to the test scripts and the new tests, removed inline from the bool variables.

[tahonermann]

Thank you for the detailed response, @mstorsjo. I understand the motivation for the changes much better now.

Actually, as far as I know, there's no way to inspect the current mode set on a file descriptor - so even if we wanted to, we can't. (And other C++ libraries such as MS STL don't inspect the mode either.)

There is a way to do so, but requires use of core CRT internal headers (corecrt_internal_lowio.h) and interfaces (_textmode) that don't have stability guarantees. So, I agree, we can't (or at least shouldn't) inspect the file mode.

Secondly, as a detail, that spec draft seems to expose the deepest native handle which seems to be meant to be a HANDLE on Windows

Ah, yes, that is correct.

Microsoft's documentation for _setmode() states:

Unicode mode is for wide print functions (for example, wprintf) and is not supported for narrow print functions. Use of a narrow print function on a Unicode mode stream triggers an assert.

That makes it clear that the standard library must restrict itself to use of the wide print functions with streams with those file modes. But since we can't check the file mode, that means programmers that (incorrectly) use std::cout following a call to _setmode() to put the stream in Unicode mode will experience an error that appears to put the blame on the standard library. Perhaps it is worth documenting these interactions.

Perhaps we should be using the wide printing functions for wchar_t streams on all platforms. wchar_t is seldom used on platforms other than Windows; do std::wcin, std::wcout, and std::wcerr actually work in a reasonable way on non-Windows platforms today?

libcxx/src/std_stream.h

128–149 ↗

(On Diff #514711)

I think it may be possible to do this in a cleaner way by factoring out the calls to getc() and getwc() doing something like this:
Add some helper methods:

static bool __do_getc(FILE* __fp, char* __pbuf) {
    int __c = getc(__fp);
    if (__c == EOF)
        return false
     *__pbuf = static_cast<char>(__c);
    return true;
}
#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
static bool __do_getc(FILE* __fp, wchar_t *pbuf) {
    wint_t __c = getwc(__file_);
    if (__c == WEOF)
        return false;
    *__pbuf = static_cast<wchar_t>(__c);
    return true;
}
#endif

Change the above code to:

char_type __extbuf[__limit];
int __nread = _VSTD::max(1, __encoding_);
for (int __i = 0; __i < __nread; ++__i)
{
    if (!do_getc(__file_, &extbuf[__i]))
        return traits_type::eof();
}

Do similarly for ungetc/ungetwc and fwrite/fputwc further below.

[mstorsjo]

In D146398#4281542, @tahonermann wrote:

Unicode mode is for wide print functions (for example, wprintf) and is not supported for narrow print functions. Use of a narrow print function on a Unicode mode stream triggers an assert.

That makes it clear that the standard library must restrict itself to use of the wide print functions with streams with those file modes. But since we can't check the file mode, that means programmers that (incorrectly) use std::cout following a call to _setmode() to put the stream in Unicode mode will experience an error that appears to put the blame on the standard library. Perhaps it is worth documenting these interactions.

Yeah, possibly - any suggestions on where?

Perhaps we should be using the wide printing functions for wchar_t streams on all platforms. wchar_t is seldom used on platforms other than Windows; do std::wcin, std::wcout, and std::wcerr actually work in a reasonable way on non-Windows platforms today?

I think the current behaviour is mostly reasonable on other platforms; currently, all actual IO towards stdin/stdout/stderr happens via narrow chars, and if using a wchar_t based C++ stream, the narrow char stream gets converted between wchar_t and utf8 in narrow chars. On Windows, you get the full unicode fidelity of the console by communicating with it in unicode mode, but actually writing wchars to it on Unix probably doesn't do any good...

libcxx/src/std_stream.h

128–149 ↗

(On Diff #514711)

Thanks for the suggestion - that would probably help a bit! We'd still need the ifdef around the char_type __extbuf[__limit]; though, as we need it as char for the non-Windows cases; the codecvt conversion in __cv_->in below only works if the source buffer is char here. But such a factorization can indeed clean up a few of the other cases of conditional code.

[tahonermann]

Yeah, possibly - any suggestions on where?

I would have to defer to @ldionne and @Mordante for that.

and if using a wchar_t based C++ stream, the narrow char stream gets converted between wchar_t and utf8 in narrow chars

Sorry, I wasn't able to parse that. I think I see what you mean though; the locale facet gets used to convert the wchar_t sequence to/from char.

On Windows, you get the full unicode fidelity of the console by communicating with it in unicode mode, but actually writing wchars to it on Unix probably doesn't do any good...

I agree writing wchar_t values directly wouldn't be good but I would expect the implementation of fputwc() to do something reasonable. However...

Hmm, is it really standards conforming to not use the std::codecvt<wchar_t, char, std::mbstate_t locale facet for std::wcout and friends? We can make an argument that a program that contains a call to _setmode() doesn't require conformance, but in the absence of such a call, I would think that we are required to use the imbued facet. As is, with these changes, if a user were to imbue their own facet, it would get ignored. Do you know how the Microsoft or libstdcxx implementations handle that?

libcxx/src/std_stream.h

128–149 ↗

(On Diff #514711)

We could factor out the use of the codecvt facet in the same way. That would avoid the need to use if constexpr for deadcode elimination too.

[mstorsjo]

In D146398#4281670, @tahonermann wrote:

and if using a wchar_t based C++ stream, the narrow char stream gets converted between wchar_t and utf8 in narrow chars

Sorry, I wasn't able to parse that. I think I see what you mean though; the locale facet gets used to convert the wchar_t sequence to/from char.

Sorry for the unclear writing - yes, that's what I meant.

On Windows, you get the full unicode fidelity of the console by communicating with it in unicode mode, but actually writing wchars to it on Unix probably doesn't do any good...

I agree writing wchar_t values directly wouldn't be good but I would expect the implementation of fputwc() to do something reasonable. However...

It does something somewhat reasonable at least; fopen(); fputwc() ... on Linux writes the wchars as plain ASCII, anything outside of the ASCII range seems to produce literal ? chars. On Windows, the same gets the wchars converted to the system's native codepage. If I fopen the file in binary mode, I get the wchars written as such, without any conversion on Windows. On Linux, opening the file in binary mode doesn't make any difference.

Hmm, is it really standards conforming to not use the std::codecvt<wchar_t, char, std::mbstate_t locale facet for std::wcout and friends? We can make an argument that a program that contains a call to _setmode() doesn't require conformance, but in the absence of such a call, I would think that we are required to use the imbued facet. As is, with these changes, if a user were to imbue their own facet, it would get ignored. Do you know how the Microsoft or libstdcxx implementations handle that?

Hmm, right - I briefly thought about that. I was pointed to e.g. https://github.com/microsoft/STL/blob/daa994bfc41c36196c536f2b68388f859d6bd656/stl/inc/fstream#L609-L634. For regular chars, there seems to be a fastpath if there's no _Pcvt, otherwise it calls _Mysb::xsputn, and for wchars it always does the latter. I think https://github.com/microsoft/STL/blob/daa994bfc41c36196c536f2b68388f859d6bd656/stl/inc/fstream#L416-L467 is what might be handling the outputting... It seems like if the _Pcvt is needed and signals that it did some conversion, it does convert to char and write that out, otherwise it keeps it in the native form and fputc/fputwcs it.

On the other hand, if it ever does that, and the stream is set to unicode mode, it would end up trying to fwrite narrow char data to a unicode stream, which doesn't really work. So perhaps that case is simply not supported?

Can you show examples on how one could experimentally try out imbuing a nondefault locale for the wcout/wcerr/wcin streams, to experimentally see how MS STL behaves in that case?

So with that in mind, I guess we should try to keep the current codepaths that do conversions but either avoid hitting them or keeping the original wchar form around if noconv is returned... I guess I should sit down with the MS STL implementation and see if I can trace what codepaths it ends up using for e.g. wcout.

[tahonermann]

It does something somewhat reasonable at least; fopen(); fputwc() ... on Linux writes the wchars as plain ASCII, anything outside of the ASCII range seems to produce literal ? chars.

The fputwc man page claims conversion to the locale encoding; which defaults to "C" unless setlocale() has been called. So ASCII by default, but maybe not.

Can you show examples on how one could experimentally try out imbuing a nondefault locale for the wcout/wcerr/wcin streams, to experimentally see how MS STL behaves in that case?

Here you go: https://godbolt.org/z/esP1535P4. Interesting, it looks like the Microsoft and libc++ implementations both call the codecvt members for each individual character while libstdcxx batches them. (I had to test MSVC locally; I look forward to someday being able to run MSVC generated code on godbolt.org again!)

So with that in mind, I guess we should try to keep the current codepaths that do conversions but either avoid hitting them or keeping the original wchar form around if noconv is returned... I guess I should sit down with the MS STL implementation and see if I can trace what codepaths it ends up using for e.g. wcout.

I would expect that noconv will never be true when the internal and external character types differ since, at a minimum, a (potentially narrowing) copy has to be performed.

[Mordante]

In D146398#4281670, @tahonermann wrote:

Yeah, possibly - any suggestions on where?

I would have to defer to @ldionne and @Mordante for that.

I think this page https://libcxx.llvm.org/UsingLibcxx.html would be a good place. Maybe put it under a new section Platform specific behavior and a subsection Windows.

[mstorsjo]

Sorry for the late followup here, I had other things I had to prioritize...

In D146398#4285271, @tahonermann wrote:

It does something somewhat reasonable at least; fopen(); fputwc() ... on Linux writes the wchars as plain ASCII, anything outside of the ASCII range seems to produce literal ? chars.

The fputwc man page claims conversion to the locale encoding; which defaults to "C" unless setlocale() has been called. So ASCII by default, but maybe not.

Oh indeed, after a setlocale() call, fputwc() does seem to work as expected, converting to the locale's charset (utf8) - the b flag to fopen() doesn't make any difference.

Can you show examples on how one could experimentally try out imbuing a nondefault locale for the wcout/wcerr/wcin streams, to experimentally see how MS STL behaves in that case?

Here you go: https://godbolt.org/z/esP1535P4. Interesting, it looks like the Microsoft and libc++ implementations both call the codecvt members for each individual character while libstdcxx batches them. (I had to test MSVC locally; I look forward to someday being able to run MSVC generated code on godbolt.org again!)

Thanks! And with that example, if I add _setmode(_fileno(stdout), _O_WTEXT);, the output gets totally garbled. If I then remove the call to imbue, the output again works as expected.

So on Windows/MS STL - by default, both cout and wcout work fine, and I can imbue a locale with a custom conversion on wcout - the conversion is honored and works. If I set stdout to unicode mode, plain cout doesn't work any longer, and also a wcout with an imbued locale, which forces a conversion to char, doesn't work either.

I guess that should be a behaviour that we should be able to match.

So with that in mind, I guess we should try to keep the current codepaths that do conversions but either avoid hitting them or keeping the original wchar form around if noconv is returned... I guess I should sit down with the MS STL implementation and see if I can trace what codepaths it ends up using for e.g. wcout.

I would expect that noconv will never be true when the internal and external character types differ since, at a minimum, a (potentially narrowing) copy has to be performed.

Right - I'll try to study this next.

[mstorsjo]

In D146398#4285271, @tahonermann wrote:

In D146398#4282999, @mstorsjo wrote:

In D146398#4281670, @tahonermann wrote:

Hmm, is it really standards conforming to not use the std::codecvt<wchar_t, char, std::mbstate_t locale facet for std::wcout and friends? We can make an argument that a program that contains a call to _setmode() doesn't require conformance, but in the absence of such a call, I would think that we are required to use the imbued facet. As is, with these changes, if a user were to imbue their own facet, it would get ignored. Do you know how the Microsoft or libstdcxx implementations handle that?

So with that in mind, I guess we should try to keep the current codepaths that do conversions but either avoid hitting them or keeping the original wchar form around if noconv is returned... I guess I should sit down with the MS STL implementation and see if I can trace what codepaths it ends up using for e.g. wcout.

I would expect that noconv will never be true when the internal and external character types differ since, at a minimum, a (potentially narrowing) copy has to be performed.

I dug into this; the main difference seems to be this:

• In STL, there's a basic_filebuf initialized here: https://github.com/microsoft/STL/blob/6d2f8b0ed88ea6cba26cc2151f47f678442c1663/stl/inc/fstream#L174-L176 Which calls _Init: https://github.com/microsoft/STL/blob/6d2f8b0ed88ea6cba26cc2151f47f678442c1663/stl/inc/fstream#L703-L728 Which sets _Pcvt to null. So by default there's no conversion set up. If the user calls imbue, https://github.com/microsoft/STL/blob/6d2f8b0ed88ea6cba26cc2151f47f678442c1663/stl/inc/fstream#L698-L701, it does set up _Pcvt: https://github.com/microsoft/STL/blob/6d2f8b0ed88ea6cba26cc2151f47f678442c1663/stl/inc/fstream#L766-L773
• In libc++, we always set up __cv_ based on this->getloc(): https://github.com/llvm/llvm-project/blob/fe3ed6550ef277f5c47a9ccdbdd466fa225f4a99/libcxx/src/std_stream.h#L251-L258 https://github.com/llvm/llvm-project/blob/fe3ed6550ef277f5c47a9ccdbdd466fa225f4a99/libcxx/src/std_stream.h#L76

So, would a conformant way forward be this?

• Keep doing whatever we're doing now, on Unix platforms
• On Windows, don't imbue this->getloc() by default, but default to always_noconv, but keep the conversion codepaths alive. If the user calls imbue(), we'd activate the __cv_ converter
• Make sure that we have a valid fastpath for always_noconv that preserves data in char_type without munging it via a char buffer, for both input and output? (We do have some such fastpaths, but they need to be tweaked to run e.g. fputwc repeatedly instead of doing one large fwrite.)

[mstorsjo]

Rebase to rerun CI after D148324, which gives more CI coverage for these tests.

The suggested refactorings and changes haven't been done yet - will try when I have more time.

[mstorsjo]

Try to restart the CI, which failed on the patching stage.

[mstorsjo]

Apply the simplifications suggested by @tahonermann.

Keep most of the codepaths as they were, doing conversions between wchar and char using codecvt. The main behaviour difference from git main is that we set __always_noconv_ to true for the __is_win32api_wide_char case. If the user imbues a custom locale, that is used and __always_noconv_ is updated based on that.

For the output case, this is mostly quite straightforward with just one small piece of Windows specific logic, to avoid using fwrite() for more than one wchar_t and fall back on repeated fputwc() instead. (fputwc() works in both the default and Unicode modes of stdout, while fwrite() with wchar_t only works if the stream has been changed to Unicode mode.)

For the input case, we have to restructure code to avoid truncating chars into an intermediate char buffer, for the __always_noconv_ case. I'm not sure how well the testsuite covers the corner cases here; I've tried to retain the old existing logic through the refactoring.

I added testcases that imbue a custom locale with a custom codecvt on the wide streams, to make sure that case still works.

I added a mention of these corner cases in the UsingLibcxx document.

[mstorsjo]

libcxx/src/std_stream.h

139 ↗

(On Diff #518684)

I'm a bit unsure about whether this is ok to do; can we generally assume that std::wint_t and int are distinct different data types? If we'd just pass the raw char/wchar_t here, we can't access traits_type::to_int_type here. Or should we do that and directly use char_traits<type>::to_int_type explicitly instead of going via the traits_type typedef within the class?

[tahonermann]

libcxx/docs/UsingLibcxx.rst

534–538 ↗

(On Diff #518684)

Microsoft's [[ https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setmode?view=msvc-170 | documentation for _setmode() ]] states that use of a narrow print function on a Unicode stream will result in an assertion failure (when linked with a debug C run-time library) and that is the case for printf() and putc(). However, it isn't the case for Microsoft's implementation of std::cout and friends; they misbehave (the narrow char buffer gets interpreted as holding a sequence of wchar_t which, of course, does nothing useful), but they don't assert. It seems they must bypass the C functions that assert; probably by calling fwrite() (which doesn't assert).

I'm wondering if we should say more about what the ramifications are if the narrow streams are used in Unicode mode; saying "can't be used" doesn't communicate much. The suggested edit attempts to better explain what happens.

540–542 ↗

(On Diff #518684)

I wonder if we should say something about std::ios::sync_with_stdio() here. Microsoft's implementation requires sync_with_stdio(false) for an imbued locale to have any effect. Is libc++ consistent with that requirement? If not, perhaps it should be?

libcxx/src/std_stream.h

139 ↗

(On Diff #518684)

Good catch; I don't think we can depend on them being distinct types. It looks like there are cases where an "int_type" value is passed, so I think we should pass the right type. Here are a few other suggestions to choose from. Since this is pretty isolated code, I don't have strong opinions at all. Just make sure the intent is either obvious in the code or add a comment otherwise.

• Pass a dummy argument solely for overload resolution.
• Make these a template and pass the character type as a template parameter.

327 ↗

(On Diff #518684)

396–398 ↗

(On Diff #518684)

[mstorsjo]

libcxx/docs/UsingLibcxx.rst

534–538 ↗

(On Diff #518684)

Thanks, the suggested text here seems fine!

540–542 ↗

(On Diff #518684)

Your suggested text here doesn't quite seem to match what we do.

On other platforms than Windows, then yes, libcxx does match the current or default locale by default. However, the core of this patch is exactly that on Windows, we _don't_ care about the current/default locale for the wide streams, we just pass things through by default (doing the writes as wchar_ts).

If we do imbue a locale, the writes end up done with chars instead, which doesn't behave as expected if the stream has been set in Unicode mode. So the combination of imbue() and _setmode() doesn't work, which was what I tried to say here before.

I took your suggestion but rewrote it to mean what I tried to say here, please have another look.

Then secondly, libcxx's sync_with_stdio() seems to be more or less a no-op so it isn't needed here right now at least. Not sure what we should say about it here...

libcxx/src/std_stream.h

139 ↗

(On Diff #518684)

I'll amend the patch with an extra dummy argument here - that's probably the most straightforward fix.

327 ↗

(On Diff #518684)

Hmm, I primarily use fwrite here since that's what was used before anyway - if we'd have used fputc for that before, I would have gone with that here.

While this is called when we write one char at a time, we also have a separate special case - if __always_noconv_ is set, we fwrite a whole array of chars too.

While this might avoid triggering asserts in the CRT, it still mostly outputs gibberish in that case. So I'm not sure if I'd comment this saying that this is explicitly done this way to make that case "work" somehow, since it doesn't quite work anyway.

396–398 ↗

(On Diff #518684)

Thanks, amending the commit with this change.

[mstorsjo]

Applied most suggestions; rewrote the paragraph about imbuing locales. Added a dummy parameter to disambiguate overloads for __do_ungetc.

[mstorsjo]

@tahonermann What do you think of this version?

[tahonermann]

Sorry for the long delay in responding.

It looks like the changes need to be rebased; libcxx/docs/UsingLibcxx.rst appears to be removing unrelated content.

I see there is an existing wclog.sh.cpp test for which "wide-mode" and "imbue" variants aren't added by this change. I'm having a hard time convincing myself that I care though; you're call whether you want to add those for completeness or call this good enough.

I think this looks good. The only thing I care about addressing before I accept the changes is the possibly unintended changes to libcxx/docs/UsingLibcxx.rst.

libcxx/src/std_stream.h

327 ↗

(On Diff #518684)

Ok. Hmm. My desire for a comment here was driven by the presence of the comment in the wchar_t overload below; the comment there implies that fputc() doesn't work in this case (or that fwrite() wouldn't for wchar_t streams). Perhaps we should use fputc() here to explicitly opt-in to that assert in order to catch incorrect use of Unicode enabled streams.

[mstorsjo]

In D146398#4391761, @tahonermann wrote:

Sorry for the long delay in responding.

No problem, thanks for getting back to it!

It looks like the changes need to be rebased; libcxx/docs/UsingLibcxx.rst appears to be removing unrelated content.

Oh, oops. That seems to have been a rebase conflict resolution error on my side, thanks for catching it! I'll update the patch to restore it properly.

I see there is an existing wclog.sh.cpp test for which "wide-mode" and "imbue" variants aren't added by this change. I'm having a hard time convincing myself that I care though; you're call whether you want to add those for completeness or call this good enough.

Oh, I had missed that one. At this point I wouldn't really bother...

I think this looks good. The only thing I care about addressing before I accept the changes is the possibly unintended changes to libcxx/docs/UsingLibcxx.rst.

Thanks!

libcxx/src/std_stream.h

327 ↗

(On Diff #518684)

Ah, I see.

I could clarify that comment like this:

// fputwc works regardless of wide/narrow mode of stdout, while
// fwrite of wchar_t only works if the stream actually has been set
// into wide mode.

I'd kinda rather not change the use of fwrite into fput for the existing narrow case; I'm mildly afraid of affecting behaviours (including performance) on other platforms. If we really do want to change that, it could be an entirely separate change (which can be reverted individually if needed), but I'd prefer to hold off of it at this point...

[mstorsjo]

Restored the rebase conflict mishap in the docs, clarified the code comment about fputwc vs fwrite.

[tahonermann]

This looks good to me. I recommend getting an approval from @Mordante or @ldionne before pushing the change though.

libcxx/src/std_stream.h

327 ↗

(On Diff #518684)

Understood. I think the updated comment is good; I'm content with this.

[Mordante]

LGTM modulo 2 nits.

libcxx/docs/UsingLibcxx.rst

565 ↗

(On Diff #527971)

libcxx/test/std/input.output/iostream.objects/wide.stream.objects/wcerr-wide-mode.sh.cpp

13 ↗

(On Diff #527971)

These tests use wide character types internally so I don't think we need to test they really fail.

[mstorsjo]

libcxx/docs/UsingLibcxx.rst

565 ↗

(On Diff #527971)

Thanks for catching, will fix.

libcxx/test/std/input.output/iostream.objects/wide.stream.objects/wcerr-wide-mode.sh.cpp

13 ↗

(On Diff #527971)

Ok, fair enough.

FWIW we've got a couple other existing tests in this directory that use XFAIL: no-wide-characters, I guess we could change that, but that'd be a new change. I'll push this with UNSUPPORTED though.

libcxx/include/__std_stream

 
 template <class _CharT>
 void
 __stdinbuf<_CharT>::imbue(const locale& __loc)
 {
     __cv_ = &use_facet<codecvt<char_type, char, state_type> >(__loc);
     __encoding_ = __cv_->encoding();
     __always_noconv_ = __cv_->always_noconv();
+#if defined(_LIBCPP_WIN32API)
+    if constexpr (sizeof(char_type) == sizeof(wchar_t)) {
+        __encoding_ = 1;
+        __always_noconv_ = true;
+    }
+#endif
     if (__encoding_ > __limit)
         __throw_runtime_error("unsupported locale for standard input");
 }
 
 template <class _CharT>
 typename __stdinbuf<_CharT>::int_type
 __stdinbuf<_CharT>::underflow()
 {
if (__last_consumed_is_next_)
         int_type __result = __last_consumed_;
         if (__consume)
         {
             __last_consumed_ = traits_type::eof();
             __last_consumed_is_next_ = false;
         }
         return __result;
     }
+#if defined(_LIBCPP_WIN32API)
+    char_type __extbuf[__limit];
+#else
     char __extbuf[__limit];
+#endif
     int __nread = _VSTD::max(1, __encoding_);
     for (int __i = 0; __i < __nread; ++__i)
     {
+#if defined(_LIBCPP_WIN32API)
+        if constexpr (sizeof(char_type) == sizeof(wchar_t)) {
+            int __c = getwc(__file_);
+            if (__c == WEOF)
+                return traits_type::eof();
+            __extbuf[__i] = static_cast<char_type>(__c);
+        } else
+#endif
+        {
             int __c = getc(__file_);
             if (__c == EOF)
                 return traits_type::eof();
             __extbuf[__i] = static_cast<char>(__c);
         }
+    }
     char_type __1buf;
     if (__always_noconv_)
         __1buf = static_cast<char_type>(__extbuf[0]);
     else
+#if defined(_LIBCPP_WIN32API)
+          // In wchar_t mode, __extbuf is the wrong type for __cv_->in below.
+          // In this mode, __always_noconv_ is true (but the compiler doesn't
+          // know), let it know that it doesn't need to instantiate this below.
+          if constexpr (sizeof(char_type) == sizeof(char))
+#endif
     {
         const char* __enxt;
         char_type* __inxt;
         codecvt_base::result __r;
         do
         {
             state_type __sv_st = *__st_;
             __r = __cv_->in(*__st_, __extbuf, __extbuf + __nread, __enxt,
#endif
                 break;
             }
         } while (__r == _VSTD::codecvt_base::partial);
     }
     if (!__consume)
     {
         for (int __i = __nread; __i > 0;)
         {
+#if defined(_LIBCPP_WIN32API)
+            if constexpr (sizeof(char_type) == sizeof(wchar_t)) {
+                if (ungetwc(traits_type::to_int_type(__extbuf[--__i]), __file_) == WEOF)
+                    return traits_type::eof();
+            } else
+#endif
             if (ungetc(traits_type::to_int_type(__extbuf[--__i]), __file_) == EOF)
                 return traits_type::eof();
         }
     }
     else
         __last_consumed_ = traits_type::to_int_type(__1buf);
     return traits_type::to_int_type(__1buf);
 }
 
 template <class _CharT>
 typename __stdinbuf<_CharT>::int_type
 __stdinbuf<_CharT>::pbackfail(int_type __c)
 {
     if (traits_type::eq_int_type(__c, traits_type::eof()))
     {
         if (!__last_consumed_is_next_)
         {
             __c = __last_consumed_;
             __last_consumed_is_next_ = !traits_type::eq_int_type(__last_consumed_,
                                                                  traits_type::eof());
         }
         return __c;
     }
+#if defined(_LIBCPP_WIN32API)
+    if (sizeof(char_type) == sizeof(wchar_t) && __last_consumed_is_next_) {
+        const char_type __ci = traits_type::to_char_type(__last_consumed_);
+        if (ungetwc(__ci, __file_) == WEOF)
+            return traits_type::eof();
+    } else
+#endif
     if (__last_consumed_is_next_)
     {
         char __extbuf[__limit];
         char* __enxt;
         const char_type __ci = traits_type::to_char_type(__last_consumed_);
         const char_type* __inxt;
         switch (__cv_->out(*__st_, &__ci, &__ci + 1, __inxt,
                                   __extbuf, __extbuf + sizeof(__extbuf), __enxt))

 
 template <class _CharT>
 __stdoutbuf<_CharT>::__stdoutbuf(FILE* __fp, state_type* __st)
     : __file_(__fp),
       __cv_(&use_facet<codecvt<char_type, char, state_type> >(this->getloc())),
       __st_(__st),
       __always_noconv_(__cv_->always_noconv())
 {
+#if defined(_LIBCPP_WIN32API)
+    if constexpr (sizeof(char_type) == sizeof(wchar_t))
+        __always_noconv_ = true;
+#endif
 }
 
 template <class _CharT>
 typename __stdoutbuf<_CharT>::int_type
 __stdoutbuf<_CharT>::overflow(int_type __c)
 {
     char __extbuf[__limit];
     char_type __1buf;
     if (!traits_type::eq_int_type(__c, traits_type::eof()))
     {
         __1buf = traits_type::to_char_type(__c);
         if (__always_noconv_)
         {
+#if defined(_LIBCPP_WIN32API)
+            if constexpr (sizeof(char_type) == sizeof(wchar_t)) {
+                // fputwc works regardless of wide/narrow mode of stdout.
+                if (fputwc(__1buf, __file_) == WEOF)
+                    return traits_type::eof();
+            } else
+#endif
             if (fwrite(&__1buf, sizeof(char_type), 1, __file_) != 1)
                 return traits_type::eof();
         }
         else
         {
             char* __extbe = __extbuf;
             codecvt_base::result __r;
             char_type* pbase = &__1buf;
#endif
     }
     return traits_type::not_eof(__c);
 }
 
 template <class _CharT>
 streamsize
 __stdoutbuf<_CharT>::xsputn(const char_type* __s, streamsize __n)
 {
-    if (__always_noconv_)
+    if (__always_noconv_
+#if defined(_LIBCPP_WIN32API)
+        // for wchar_t, prefer writing it element by element with fputwc,
+        // since that works when stdout is in narrow mode too.
+        && sizeof(char_type) != sizeof(wchar_t)
+#endif
+    )
         return fwrite(__s, sizeof(char_type), __n, __file_);
     streamsize __i = 0;
     for (; __i < __n; ++__i, ++__s)
         if (overflow(traits_type::to_int_type(*__s)) == traits_type::eof())
             break;
     return __i;
 }
 

 
 template <class _CharT>
 void
 __stdoutbuf<_CharT>::imbue(const locale& __loc)
 {
     sync();
     __cv_ = &use_facet<codecvt<char_type, char, state_type> >(__loc);
     __always_noconv_ = __cv_->always_noconv();
+#if defined(_LIBCPP_WIN32API)
+    if constexpr (sizeof(char_type) == sizeof(wchar_t))
+        __always_noconv_ = true;
+#endif
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP___STD_STREAM