From 363e6f8ec658c840ee8e7906dd4fa83a8a07e2ee Mon Sep 17 00:00:00 2001 From: Dimitrij Mijoski Date: Tue, 14 May 2024 13:02:50 +0200 Subject: [PATCH 1/3] [libc++] Fix unnecessary flushes in std::print() on POSIX The check whether the stream is associated with a terminal or not is needed only on Windows. The flush is needed only on Windows and when the stream is terminal. The flushing is done only once before using the native Unicode API on Windows. Additionally, the correct flush is now called. In the case of C stream overloads of print(), std::fflush() should be used, and in the ostream overloads, only ostream::flush() member function should be used. Before this fix, the ostream overloads called ostream::flush() and then std::fflush(). See also https://wg21.link/LWG4044. Fixes #70142 --- libcxx/include/__ostream/print.h | 8 +- libcxx/include/print | 27 +++---- .../vprint_unicode.pass.cpp | 28 +++++++ .../print.fun/vprint_unicode_posix.pass.cpp | 79 ------------------- .../print.fun/vprint_unicode_windows.pass.cpp | 30 +------ 5 files changed, 43 insertions(+), 129 deletions(-) delete mode 100644 libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp diff --git a/libcxx/include/__ostream/print.h b/libcxx/include/__ostream/print.h index eb4233342214d..f55497809d8b3 100644 --- a/libcxx/include/__ostream/print.h +++ b/libcxx/include/__ostream/print.h @@ -97,7 +97,7 @@ _LIBCPP_EXPORTED_FROM_ABI FILE* __get_ostream_file(ostream& __os); # if _LIBCPP_HAS_UNICODE template // TODO PRINT template or availability markup fires too eagerly (http://llvm.org/PR61563). _LIBCPP_HIDE_FROM_ABI void __vprint_unicode(ostream& __os, string_view __fmt, format_args __args, bool __write_nl) { -# if _LIBCPP_AVAILABILITY_HAS_PRINT == 0 +# if _LIBCPP_AVAILABILITY_HAS_PRINT == 0 || !defined(_LIBCPP_WIN32API) return std::__vprint_nonunicode(__os, __fmt, __args, __write_nl); # else FILE* __file = std::__get_ostream_file(__os); @@ -120,10 +120,8 @@ _LIBCPP_HIDE_FROM_ABI void __vprint_unicode(ostream& __os, string_view __fmt, fo # endif // _LIBCPP_HAS_EXCEPTIONS ostream::sentry __s(__os); if (__s) { -# ifndef _LIBCPP_WIN32API - __print::__vprint_unicode_posix(__file, __fmt, __args, __write_nl, true); -# elif _LIBCPP_HAS_WIDE_CHARACTERS - __print::__vprint_unicode_windows(__file, __fmt, __args, __write_nl, true); +# if _LIBCPP_HAS_WIDE_CHARACTERS + __print::__vprint_unicode_windows(__file, __fmt, __args, __write_nl); # else # error "Windows builds with wchar_t disabled are not supported." # endif diff --git a/libcxx/include/print b/libcxx/include/print index 8a8b686d18f34..6856e33245c51 100644 --- a/libcxx/include/print +++ b/libcxx/include/print @@ -233,26 +233,11 @@ __vprint_nonunicode(FILE* __stream, string_view __fmt, format_args __args, bool // terminal when the output is redirected. Typically during testing the // output is redirected to be able to capture it. This makes it hard to // test this code path. -template // TODO PRINT template or availability markup fires too eagerly (http://llvm.org/PR61563). -_LIBCPP_HIDE_FROM_ABI inline void -__vprint_unicode_posix(FILE* __stream, string_view __fmt, format_args __args, bool __write_nl, bool __is_terminal) { - // TODO PRINT Should flush errors throw too? - if (__is_terminal) - std::fflush(__stream); - - __print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl); -} # if _LIBCPP_HAS_WIDE_CHARACTERS template // TODO PRINT template or availability markup fires too eagerly (http://llvm.org/PR61563). _LIBCPP_HIDE_FROM_ABI inline void -__vprint_unicode_windows(FILE* __stream, string_view __fmt, format_args __args, bool __write_nl, bool __is_terminal) { - if (!__is_terminal) - return __print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl); - - // TODO PRINT Should flush errors throw too? - std::fflush(__stream); - +__vprint_unicode_windows([[maybe_unused]] FILE* __stream, string_view __fmt, format_args __args, bool __write_nl) { string __str = std::vformat(__fmt, __args); // UTF-16 uses the same number or less code units than UTF-8. // However the size of the code unit is 16 bits instead of 8 bits. @@ -313,9 +298,15 @@ __vprint_unicode([[maybe_unused]] FILE* __stream, // Windows there is a different API. This API requires transcoding. # ifndef _LIBCPP_WIN32API - __print::__vprint_unicode_posix(__stream, __fmt, __args, __write_nl, __print::__is_terminal(__stream)); + __print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl); # elif _LIBCPP_HAS_WIDE_CHARACTERS - __print::__vprint_unicode_windows(__stream, __fmt, __args, __write_nl, __print::__is_terminal(__stream)); + if (__print::__is_terminal(__stream)) { + // TODO PRINT Should flush errors throw too? + std::fflush(__stream); + __print::__vprint_unicode_windows(__stream, __fmt, __args, __write_nl); + } else { + __print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl); + } # else # error "Windows builds with wchar_t disabled are not supported." # endif diff --git a/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp b/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp index 52d8500f7fa3a..7882d237f8f20 100644 --- a/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp +++ b/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp @@ -81,14 +81,22 @@ static void test_is_terminal_file_stream() { assert(stream.is_open()); assert(stream.good()); std::print(stream, "test"); +#ifdef _WIN32 assert(is_terminal_calls == 1); +#else + assert(is_terminal_calls == 0); +#endif } { std::ofstream stream(filename); assert(stream.is_open()); assert(stream.good()); std::print(stream, "test"); +#ifdef _WIN32 assert(is_terminal_calls == 2); +#else + assert(is_terminal_calls == 0); +#endif } } @@ -105,7 +113,11 @@ static void test_is_terminal_rdbuf_derived_from_filebuf() { std::ostream stream(&buf); std::print(stream, "test"); +#ifdef _WIN32 assert(is_terminal_calls == 1); +#else + assert(is_terminal_calls == 0); +#endif } // When the stream is cout, clog, or cerr, its FILE* may be a terminal. Validate @@ -115,15 +127,27 @@ static void test_is_terminal_std_cout_cerr_clog() { is_terminal_result = false; { std::print(std::cout, "test"); +#ifdef _WIN32 assert(is_terminal_calls == 1); +#else + assert(is_terminal_calls == 0); +#endif } { std::print(std::cerr, "test"); +#ifdef _WIN32 assert(is_terminal_calls == 2); +#else + assert(is_terminal_calls == 0); +#endif } { std::print(std::clog, "test"); +#ifdef _WIN32 assert(is_terminal_calls == 3); +#else + assert(is_terminal_calls == 0); +#endif } } @@ -156,7 +180,11 @@ static void test_is_terminal_is_flushed() { // A terminal sync is called. is_terminal_result = true; std::print(stream, ""); +#ifdef _WIN32 assert(buf.sync_calls == 1); // only called from the destructor of the sentry +#else + assert(buf.sync_calls == 0); +#endif } int main(int, char**) { diff --git a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp b/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp deleted file mode 100644 index b89d02ba99425..0000000000000 --- a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp +++ /dev/null @@ -1,79 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// 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 -// -//===----------------------------------------------------------------------===// - -// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20 -// UNSUPPORTED: no-filesystem -// UNSUPPORTED: libcpp-has-no-unicode -// UNSUPPORTED: GCC-ALWAYS_INLINE-FIXME - -// XFAIL: availability-fp_to_chars-missing - -// fmemopen is available starting in Android M (API 23) -// XFAIL: target={{.+}}-android{{(eabi)?(21|22)}} - -// REQUIRES: has-unix-headers - -// - -// Tests the implementation of -// void __print::__vprint_unicode_posix(FILE* __stream, string_view __fmt, -// format_args __args, bool __write_nl, -// bool __is_terminal); -// -// In the library when the stdout is redirected to a file it is no -// longer considered a terminal and the special terminal handling is no -// longer executed. By testing this function we can "force" emulate a -// terminal. -// Note __write_nl is tested by the public API. - -#include -#include -#include -#include -#include - -#include "test_macros.h" - -int main(int, char**) { - std::array buffer; - std::ranges::fill(buffer, '*'); - - FILE* file = fmemopen(buffer.data(), buffer.size(), "wb"); - assert(file); - - // Test the file is buffered. - std::fprintf(file, "Hello"); - assert(std::ftell(file) == 5); -#if defined(TEST_HAS_GLIBC) && \ - !(__has_feature(address_sanitizer) || __has_feature(thread_sanitizer) || __has_feature(memory_sanitizer)) - assert(std::ranges::all_of(buffer, [](char c) { return c == '*'; })); -#endif - - // Test writing to a "non-terminal" stream does not flush. - std::__print::__vprint_unicode_posix(file, " world", std::make_format_args(), false, false); - assert(std::ftell(file) == 11); -#if defined(TEST_HAS_GLIBC) && \ - !(__has_feature(address_sanitizer) || __has_feature(thread_sanitizer) || __has_feature(memory_sanitizer)) - assert(std::ranges::all_of(buffer, [](char c) { return c == '*'; })); -#endif - - // Test writing to a "terminal" stream flushes before writing. - std::__print::__vprint_unicode_posix(file, "!", std::make_format_args(), false, true); - assert(std::ftell(file) == 12); - assert(std::string_view(buffer.data(), buffer.data() + 11) == "Hello world"); -#if defined(TEST_HAS_GLIBC) - // glibc does not flush after a write. - assert(buffer[11] != '!'); -#endif - - // Test everything is written when closing the stream. - std::fclose(file); - assert(std::string_view(buffer.data(), buffer.data() + 12) == "Hello world!"); - - return 0; -} diff --git a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp b/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp index bcd1d05a3aeeb..4777d9a3180f4 100644 --- a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp +++ b/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp @@ -22,7 +22,7 @@ // Tests the implementation of // void __print::__vprint_unicode_windows(FILE* __stream, string_view __fmt, // format_args __args, bool __write_nl, -// bool __is_terminal); +// ); // // In the library when the stdout is redirected to a file it is no // longer considered a terminal and the special terminal handling is no @@ -59,43 +59,19 @@ scoped_test_env env; std::string filename = env.create_file("output.txt"); static void test_basics() { - FILE* file = std::fopen(filename.c_str(), "wb"); - assert(file); - - // Test writing to a "non-terminal" stream does not call WriteConsoleW. - std::__print::__vprint_unicode_windows(file, "Hello", std::make_format_args(), false, false); - assert(std::ftell(file) == 5); - // It's not possible to reliably test whether writing to a "terminal" stream // flushes before writing. Testing flushing a closed stream worked on some // platforms, but was unreliable. calling = true; - std::__print::__vprint_unicode_windows(file, " world", std::make_format_args(), false, true); + std::__print::__vprint_unicode_windows(stdout, " world", std::make_format_args(), false); } // When the output is a file the data is written as-is. // When the output is a "terminal" invalid UTF-8 input is flagged. static void test(std::wstring_view output, std::string_view input) { - // *** File *** - FILE* file = std::fopen(filename.c_str(), "wb"); - assert(file); - - std::__print::__vprint_unicode_windows(file, input, std::make_format_args(), false, false); - assert(std::ftell(file) == static_cast(input.size())); - std::fclose(file); - - file = std::fopen(filename.c_str(), "rb"); - assert(file); - - std::vector buffer(input.size()); - size_t read = fread(buffer.data(), 1, buffer.size(), file); - assert(read == input.size()); - assert(input == std::string_view(buffer.begin(), buffer.end())); - std::fclose(file); - // *** Terminal *** expected = output; - std::__print::__vprint_unicode_windows(file, input, std::make_format_args(), false, true); + std::__print::__vprint_unicode_windows(stdout, input, std::make_format_args(), false); } static void test() { From f7a6a0ae6ed939b1253e8a189bf71a5b0d39ecbe Mon Sep 17 00:00:00 2001 From: Dimitrij Mijoski Date: Tue, 14 May 2024 18:12:49 +0200 Subject: [PATCH 2/3] fixup! [libc++] Fix unnecessary flushes in std::print() on POSIX Fix test for modules --- .../ostream.formatted.print/vprint_unicode.pass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp b/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp index 7882d237f8f20..c3c60ed705361 100644 --- a/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp +++ b/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp @@ -19,7 +19,7 @@ // XFAIL: availability-print-missing // Clang modules do not work with the definiton of _LIBCPP_TESTING_PRINT_IS_TERMINAL -// XFAIL: clang-modules-build +// XFAIL: clang-modules-build && target={{.*}}-windows{{.*}} // // Tests the implementation of From a9d9b5c95d701badbde1279615ffd256aa0273ba Mon Sep 17 00:00:00 2001 From: Dimitrij Mijoski Date: Tue, 23 Jul 2024 13:23:19 +0200 Subject: [PATCH 3/3] fixup! [libc++] Fix unnecessary flushes in std::print() on POSIX Fix for CI job Apple system configuration --- .../ostream.formatted.print/vprint_unicode.pass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp b/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp index c3c60ed705361..20bccf342f165 100644 --- a/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp +++ b/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp @@ -16,7 +16,7 @@ // When std::print is unavailable, we don't rely on an implementation of // std::__is_terminal and we always assume a non-unicode and non-terminal // output. -// XFAIL: availability-print-missing +// XFAIL: availability-print-missing && target={{.*}}-windows{{.*}} // Clang modules do not work with the definiton of _LIBCPP_TESTING_PRINT_IS_TERMINAL // XFAIL: clang-modules-build && target={{.*}}-windows{{.*}}