[PATCH v3 0/3] MR5462: Ucrtbase's stderr is now always unbuffered.
Starting with late Windows 10 version, in ucrtbase, stderr is now unbuffered whatever the type of the underlying fd (previously, native only set it to unbuffered when attached to character fd (console, NUL...)). This serie adds also tests for msvcrt & ucrtbase to show the discrepancies. Note: ucrtbase's tests also include a reversed engineered structure layout for FILE. _get_stream_buffer_pointers already gives base, ptr and cnt, and toying with setvbuf gave easily the rest of the fields. Didn't spend time in guessing the flags meaning: which look different from msvcrt's. Related to https://bugs.winehq.org/show_bug.cgi?id=56459 @piotr: I can share the details (the access to the test result in the bug report is limited). -- v3: ucrtbase: Let stderr be always be unbuffered. ucrtbase/tests: Add tests for checking buffering state of standard streams. msvcrt/tests: Add tests for check buffering state of standard streams. https://gitlab.winehq.org/wine/wine/-/merge_requests/5462
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/msvcrt/tests/file.c | 62 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/dlls/msvcrt/tests/file.c b/dlls/msvcrt/tests/file.c index 1ac6cfe3182..7db5ed981d3 100644 --- a/dlls/msvcrt/tests/file.c +++ b/dlls/msvcrt/tests/file.c @@ -3059,6 +3059,67 @@ static void test_ioinfo_flags(void) free(tempf); } +static void test_std_stream_buffering(void) +{ + int dup_fd, ret, pos; + FILE *file; + char ch; + + dup_fd = _dup(STDOUT_FILENO); + if (dup_fd != -1) + { + file = freopen("std_stream_test.tmp", "w", stdout); + ok(file != NULL, "freopen failed\n"); + + ret = fprintf(stdout, "test"); + pos = _telli64(STDOUT_FILENO); + + fflush(stdout); + _dup2(dup_fd, STDOUT_FILENO); + close(dup_fd); + setvbuf(stdout, NULL, _IONBF, 0); + + ok(ret == 4, "fprintf(stdout) returned %d\n", ret); + ok(!pos, "expected stdout to be buffered\n"); + } + + dup_fd = _dup(STDERR_FILENO); + if (dup_fd != -1) + { + file = freopen("std_stream_test.tmp", "w", stderr); + ok(file != NULL, "freopen failed\n"); + + ret = fprintf(stderr, "test"); + ok(ret == 4, "fprintf(stderr) returned %d\n", ret); + pos = _telli64(STDERR_FILENO); + ok(!pos, "expected stderr to be buffered\n"); + + fflush(stderr); + _dup2(dup_fd, STDERR_FILENO); + close(dup_fd); + } + + dup_fd = _dup(STDIN_FILENO); + if (dup_fd != -1) + { + file = freopen("std_stream_test.tmp", "r", stdin); + ok(file != NULL, "freopen failed\n"); + + ch = 0; + ret = fscanf(stdin, "%c", &ch); + ok(ret == 1, "fscanf returned %d\n", ret); + ok(ch == 't', "ch = 0x%x\n", (unsigned char)ch); + pos = _telli64(STDIN_FILENO); + ok(pos == 4, "pos = %d\n", pos); + + fflush(stdin); + _dup2(dup_fd, STDIN_FILENO); + close(dup_fd); + } + + ok(DeleteFileA("std_stream_test.tmp"), "DeleteFile failed\n"); +} + START_TEST(file) { int arg_c; @@ -3135,6 +3196,7 @@ START_TEST(file) test_fopen_hints(); test_open_hints(); test_ioinfo_flags(); + test_std_stream_buffering(); /* Wait for the (_P_NOWAIT) spawned processes to finish to make sure the report * file contains lines in the correct order -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5462
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/ucrtbase/tests/Makefile.in | 1 + dlls/ucrtbase/tests/file.c | 89 +++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 dlls/ucrtbase/tests/file.c diff --git a/dlls/ucrtbase/tests/Makefile.in b/dlls/ucrtbase/tests/Makefile.in index cd01a3aacb9..ea9e589113c 100644 --- a/dlls/ucrtbase/tests/Makefile.in +++ b/dlls/ucrtbase/tests/Makefile.in @@ -5,6 +5,7 @@ EXTRADEFS = -fno-builtin SOURCES = \ cpp.c \ environ.c \ + file.c \ misc.c \ printf.c \ scanf.c \ diff --git a/dlls/ucrtbase/tests/file.c b/dlls/ucrtbase/tests/file.c new file mode 100644 index 00000000000..23f3fd62e37 --- /dev/null +++ b/dlls/ucrtbase/tests/file.c @@ -0,0 +1,89 @@ +/* + * Unit test suite for file functions + * + * Copyright 2024 Eric Pouech for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include "wine/test.h" + +static void test_std_stream_buffering(void) +{ + int dup_fd, ret, pos; + FILE *file; + char ch; + + dup_fd = _dup(STDOUT_FILENO); + if (dup_fd != -1) + { + file = freopen("std_stream_test.tmp", "w", stdout); + ok(file != NULL, "freopen failed\n"); + + ret = fprintf(stdout, "test"); + pos = _telli64(STDOUT_FILENO); + + fflush(stdout); + _dup2(dup_fd, STDOUT_FILENO); + close(dup_fd); + setvbuf(stdout, NULL, _IONBF, 0); + + ok(ret == 4, "fprintf(stdout) returned %d\n", ret); + ok(!pos, "expected stdout to be buffered\n"); + } + + dup_fd = _dup(STDERR_FILENO); + if (dup_fd != -1) + { + file = freopen("std_stream_test.tmp", "w", stderr); + ok(file != NULL, "freopen failed\n"); + + ret = fprintf(stderr, "test"); + ok(ret == 4, "fprintf(stderr) returned %d\n", ret); + pos = _telli64(STDERR_FILENO); + todo_wine + ok(pos == 4 || broken(!GetProcAddress(GetModuleHandleA("ucrtbase"), "__CxxFrameHandler4") && !pos), + "expected stderr to be unbuffered (%d)\n", pos); + + fflush(stderr); + _dup2(dup_fd, STDERR_FILENO); + close(dup_fd); + } + + dup_fd = _dup(STDIN_FILENO); + if (dup_fd != -1) + { + file = freopen("std_stream_test.tmp", "r", stdin); + ok(file != NULL, "freopen failed\n"); + + ch = 0; + ret = fscanf(stdin, "%c", &ch); + ok(ret == 1, "fscanf returned %d\n", ret); + ok(ch == 't', "ch = 0x%x\n", (unsigned char)ch); + pos = _telli64(STDIN_FILENO); + ok(pos == 4, "pos = %d\n", pos); + + fflush(stdin); + _dup2(dup_fd, STDIN_FILENO); + close(dup_fd); + } + + ok(DeleteFileA("std_stream_test.tmp"), "DeleteFile failed\n"); +} + +START_TEST(file) +{ + test_std_stream_buffering(); +} -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5462
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/msvcrt/file.c | 6 ++++++ dlls/ucrtbase/tests/file.c | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c index a3a872d4be3..24cca6fb63e 100644 --- a/dlls/msvcrt/file.c +++ b/dlls/msvcrt/file.c @@ -830,9 +830,15 @@ int CDECL _isatty(int fd) /* INTERNAL: Allocate stdio file buffer */ static BOOL msvcrt_alloc_buffer(FILE* file) { +#if _MSVCR_VER >= 140 + if((file->_file==STDOUT_FILENO && _isatty(file->_file)) + || file->_file == STDERR_FILENO) + return FALSE; +#else if((file->_file==STDOUT_FILENO || file->_file==STDERR_FILENO) && _isatty(file->_file)) return FALSE; +#endif file->_base = calloc(1, MSVCRT_INTERNAL_BUFSIZ); if(file->_base) { diff --git a/dlls/ucrtbase/tests/file.c b/dlls/ucrtbase/tests/file.c index 23f3fd62e37..cfc24594db3 100644 --- a/dlls/ucrtbase/tests/file.c +++ b/dlls/ucrtbase/tests/file.c @@ -53,7 +53,6 @@ static void test_std_stream_buffering(void) ret = fprintf(stderr, "test"); ok(ret == 4, "fprintf(stderr) returned %d\n", ret); pos = _telli64(STDERR_FILENO); - todo_wine ok(pos == 4 || broken(!GetProcAddress(GetModuleHandleA("ucrtbase"), "__CxxFrameHandler4") && !pos), "expected stderr to be unbuffered (%d)\n", pos); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5462
I've pushed a new version of the patch. I have changed the test to run in main process and restore stream when done. I haven't verified if the broken check is good enough but the tests are passing on all testbot machines (I think it can be addressed later if there's ucrtbase varsion not covered by this check). Please let me know if it looks good for you. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5462#note_67274
eric pouech (@epo) commented about dlls/msvcrt/tests/file.c:
free(tempf); }
+static void test_std_stream_buffering(void) +{ + int dup_fd, ret, pos; + FILE *file; + char ch; + + dup_fd = _dup(STDOUT_FILENO); + if (dup_fd != -1)
perhaps add an ok() test here (failure is unlikely, but should be caught IMO) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5462#note_67319
eric pouech (@epo) commented about dlls/ucrtbase/tests/file.c:
+ setvbuf(stdout, NULL, _IONBF, 0); + + ok(ret == 4, "fprintf(stdout) returned %d\n", ret); + ok(!pos, "expected stdout to be buffered\n"); + } + + dup_fd = _dup(STDERR_FILENO); + if (dup_fd != -1) + { + file = freopen("std_stream_test.tmp", "w", stderr); + ok(file != NULL, "freopen failed\n"); + + ret = fprintf(stderr, "test"); + ok(ret == 4, "fprintf(stderr) returned %d\n", ret); + pos = _telli64(STDERR_FILENO); + ok(pos == 4 || broken(!GetProcAddress(GetModuleHandleA("ucrtbase"), "__CxxFrameHandler4") && !pos), this will only be tested on x86_64 (the assumption that the buffering is orthogonal to the machine seems reasonable, but remains to be tested for future native ucrtbase versions) perhaps we could do something like:
if (broken(!GetProcAddress(GetModuleHandleA("ucrtbase"), "__CxxFrameHandler4") && !pos))
trace("stderr is unbuffered\n");
else
ok(pos == 4, ...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5462#note_67320
On Tue Apr 9 07:20:14 2024 +0000, eric pouech wrote:
perhaps add an ok() test here (failure is unlikely, but should be caught IMO) That makes sense, probably `if` is not needed at all.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5462#note_67340
On Tue Apr 9 07:20:15 2024 +0000, eric pouech wrote:
this will only be tested on x86_64 (the assumption that the buffering is orthogonal to the machine seems reasonable, but remains to be tested for future native ucrtbase versions) perhaps we could do something like: ``` if (broken(!GetProcAddress(GetModuleHandleA("ucrtbase"), "__CxxFrameHandler4") && !pos)) trace("stderr is unbuffered\n"); else ok(pos == 4, ... ``` I guess it will not be spotted until there's a failing test. If you prefer adding a trace - it works for me. Could you please update the patch?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5462#note_67341
On Tue Apr 9 10:08:51 2024 +0000, Piotr Caban wrote:
I guess it will not be spotted until there's a failing test. If you prefer adding a trace - it works for me. Could you please update the patch? ok I'll do
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5462#note_67343
participants (3)
-
Eric Pouech -
eric pouech (＠epo)
-
Piotr Caban (＠piotr)