[PATCH 0/2] MR7169: ntdll: Fix the return value of NtDelayExecution.
The key change is to never return STATUS_TIMEOUT, and to instead return the result of NtYieldExecution() if zero timeout was passed, or STATUS_SUCCESS otherwise. An overview of the correct values for each combination, copied from the test commit: - Non-alertable, zero timeout: STATUS_SUCCESS or STATUS_NO_YIELD_PERFORMED - Non-alertable, non-zero timeout: STATUS_SUCCESS - Alertable, zero timeout: STATUS_SUCCESS, STATUS_NO_YIELD_PERFORMED, or STATUS_USER_APC - Alertable, non-zero timeout: STATUS_SUCCESS or STATUS_USER_APC -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169
From: William Horvath <william(a)horvath.blog> --- dlls/ntdll/tests/sync.c | 185 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 3ab89bbf361..15f692e67bd 100644 --- a/dlls/ntdll/tests/sync.c +++ b/dlls/ntdll/tests/sync.c @@ -32,6 +32,7 @@ static NTSTATUS (WINAPI *pNtCreateEvent) ( PHANDLE, ACCESS_MASK, const OBJECT_AT static NTSTATUS (WINAPI *pNtCreateKeyedEvent)( HANDLE *, ACCESS_MASK, const OBJECT_ATTRIBUTES *, ULONG ); static NTSTATUS (WINAPI *pNtCreateMutant)( HANDLE *, ACCESS_MASK, const OBJECT_ATTRIBUTES *, BOOLEAN ); static NTSTATUS (WINAPI *pNtCreateSemaphore)( HANDLE *, ACCESS_MASK, const OBJECT_ATTRIBUTES *, LONG, LONG ); +static NTSTATUS (WINAPI *pNtDelayExecution)( BOOLEAN, const LARGE_INTEGER * ); static NTSTATUS (WINAPI *pNtOpenEvent)( HANDLE *, ACCESS_MASK, const OBJECT_ATTRIBUTES * ); static NTSTATUS (WINAPI *pNtOpenKeyedEvent)( HANDLE *, ACCESS_MASK, const OBJECT_ATTRIBUTES * ); static NTSTATUS (WINAPI *pNtPulseEvent)( HANDLE, LONG * ); @@ -1030,6 +1031,188 @@ static void test_completion_port_scheduling(void) } } +/* An overview of possible combinations and return values: + * - Non-alertable, zero timeout: STATUS_SUCCESS or STATUS_NO_YIELD_PERFORMED + * - Non-alertable, non-zero timeout: STATUS_SUCCESS + * - Alertable, zero timeout: STATUS_SUCCESS, STATUS_NO_YIELD_PERFORMED, or STATUS_USER_APC + * - Alertable, non-zero timeout: STATUS_SUCCESS or STATUS_USER_APC + */ + +struct test_delay_param +{ + HANDLE ready, completion; + volatile LONG apc_count; + NTSTATUS status; + BOOLEAN non_zero_timeout; +}; + +static VOID CALLBACK apc_func( ULONG_PTR arg ) +{ + struct test_delay_param *p = (struct test_delay_param *)arg; + InterlockedIncrement( &p->apc_count ); +} + +#define NUM_APCS 64 +#define NUM_ITERATIONS 32 + +static DWORD WINAPI delay_thread( void *arg ) +{ + struct test_delay_param *p = arg; + LARGE_INTEGER timeout = {{0}}; + NTSTATUS status; + DWORD ret; + int i; + + status = pNtDelayExecution( TRUE, &timeout ); + p->status = status; + SetEvent( p->ready ); + + ret = WaitForSingleObject( p->completion, INFINITE ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + + if (p->non_zero_timeout) + timeout.QuadPart = -10000; + + for (i = 0; i < NUM_ITERATIONS; i++) + { + status = pNtDelayExecution( TRUE, &timeout ); + if (!p->non_zero_timeout) + { + todo_wine_if(status == STATUS_TIMEOUT) + ok( status == STATUS_USER_APC || status == STATUS_NO_YIELD_PERFORMED || + status == STATUS_SUCCESS, "NtDelayExecution iteration %d returned %#lx\n", + i, status ); + } + else + { + todo_wine_if(status == STATUS_TIMEOUT) + ok( status == STATUS_USER_APC || status == STATUS_SUCCESS, + "NtDelayExecution iteration %d with timeout returned %#lx\n", + i, status ); + } + + if ((i % 10) == 0) + Sleep( 1 ); + } + + ok( p->apc_count > 0, "no APCs were delivered\n" ); + ok( p->apc_count <= NUM_APCS, "got %ld APCs, expected at most %d\n", + p->apc_count, NUM_APCS ); + + return 0; +} + +static void test_delayexecution(void) +{ + static const struct test + { + LONGLONG timeout; + BOOLEAN alertable; + } + tests[] = + { + { 0, FALSE }, + { 0, TRUE }, + { 1 * -10000, FALSE }, + { 1 * -10000, TRUE }, + { 10 * -10000, TRUE }, + }; + + struct test_delay_param param = {0}; + struct test_delay_param param_timeout = {0}; + unsigned int total_noyields[2] = {0, 0}; + LARGE_INTEGER timeout; + HANDLE thread, thread_timeout; + NTSTATUS status; + DWORD tid, tid_timeout, ret; + unsigned int i, j; + + for (i = 0; i < NUM_ITERATIONS; i++) + { + for (j = 0; j < ARRAY_SIZE(tests); j++) + { + timeout.QuadPart = tests[j].timeout; + status = pNtDelayExecution( tests[j].alertable, &timeout ); + + if (!tests[j].alertable) + { + ok( status == STATUS_SUCCESS || status == STATUS_NO_YIELD_PERFORMED, + "test %u: got %#lx, expected SUCCESS or NO_YIELD_PERFORMED\n", j, status ); + if (status == STATUS_NO_YIELD_PERFORMED) total_noyields[0]++; + } + else if (!timeout.QuadPart) + { + todo_wine_if(status == STATUS_TIMEOUT) + ok( status == STATUS_SUCCESS || status == STATUS_NO_YIELD_PERFORMED || status == STATUS_USER_APC, + "test %u: got %#lx, expected SUCCESS, NO_YIELD_PERFORMED, or USER_APC\n", j, status ); + if (status == STATUS_NO_YIELD_PERFORMED) total_noyields[1]++; + } + else + { + todo_wine_if(status == STATUS_TIMEOUT) + ok( status == STATUS_SUCCESS || status == STATUS_USER_APC, + "test %u: got %#lx, expected SUCCESS or USER_APC\n", j, status ); + } + } + } + + todo_wine ok( total_noyields[0] > 0, "Expected > 0 STATUS_NO_YIELD_PERFORMED results for non-alertable zero-timeout delays.\n" ); + todo_wine ok( total_noyields[1] > 0, "Expected > 0 STATUS_NO_YIELD_PERFORMED results for alertable zero-timeout delays.\n" ); + + param.ready = CreateEventA( NULL, FALSE, FALSE, NULL ); + ok( param.ready != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); + param.completion = CreateEventA( NULL, FALSE, FALSE, NULL ); + ok( param.completion != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); + param.non_zero_timeout = FALSE; + + param_timeout = param; + param_timeout.ready = CreateEventA( NULL, FALSE, FALSE, NULL ); + ok( param_timeout.ready != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); + param_timeout.completion = CreateEventA( NULL, FALSE, FALSE, NULL ); + ok( param_timeout.completion != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); + param_timeout.non_zero_timeout = TRUE; + + thread = CreateThread( NULL, 0, delay_thread, ¶m, 0, &tid ); + ok( thread != NULL, "CreateThread failed, error %lu\n", GetLastError() ); + thread_timeout = CreateThread( NULL, 0, delay_thread, ¶m_timeout, 0, &tid_timeout ); + ok( thread_timeout != NULL, "CreateThread failed, error %lu\n", GetLastError() ); + + ret = WaitForSingleObject( param.ready, 5000 ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + ret = WaitForSingleObject( param_timeout.ready, 5000 ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + + todo_wine + ok( param.status == STATUS_SUCCESS || param.status == STATUS_NO_YIELD_PERFORMED, + "thread NtDelayExecution returned %#lx\n", param.status ); + todo_wine + ok( param_timeout.status == STATUS_SUCCESS || param_timeout.status == STATUS_NO_YIELD_PERFORMED, + "thread NtDelayExecution (timeout) returned %#lx\n", param_timeout.status ); + + for (i = 0; i < NUM_APCS; i++) + { + ok( QueueUserAPC( apc_func, thread, (ULONG_PTR)¶m ), + "QueueUserAPC failed for zero timeout thread, error %lu\n", GetLastError() ); + ok( QueueUserAPC( apc_func, thread_timeout, (ULONG_PTR)¶m_timeout ), + "QueueUserAPC failed for non-zero timeout thread, error %lu\n", GetLastError() ); + } + + SetEvent( param.completion ); + SetEvent( param_timeout.completion ); + + ret = WaitForSingleObject( thread, 5000 ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + ret = WaitForSingleObject( thread_timeout, 5000 ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + + CloseHandle( thread ); + CloseHandle( thread_timeout ); + CloseHandle( param.ready ); + CloseHandle( param.completion ); + CloseHandle( param_timeout.ready ); + CloseHandle( param_timeout.completion ); +} + START_TEST(sync) { HMODULE module = GetModuleHandleA("ntdll.dll"); @@ -1046,6 +1229,7 @@ START_TEST(sync) pNtCreateKeyedEvent = (void *)GetProcAddress(module, "NtCreateKeyedEvent"); pNtCreateMutant = (void *)GetProcAddress(module, "NtCreateMutant"); pNtCreateSemaphore = (void *)GetProcAddress(module, "NtCreateSemaphore"); + pNtDelayExecution = (void *)GetProcAddress(module, "NtDelayExecution"); pNtOpenEvent = (void *)GetProcAddress(module, "NtOpenEvent"); pNtOpenKeyedEvent = (void *)GetProcAddress(module, "NtOpenKeyedEvent"); pNtPulseEvent = (void *)GetProcAddress(module, "NtPulseEvent"); @@ -1078,4 +1262,5 @@ START_TEST(sync) test_resource(); test_tid_alert( argv ); test_completion_port_scheduling(); + test_delayexecution(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7169
From: William Horvath <william(a)horvath.blog> The key change is to never return STATUS_TIMEOUT, and to instead return the result of NtYieldExecution() if zero timeout was passed, or STATUS_SUCCESS otherwise. An overview of the correct values for each combination, copied from the test commit: - Non-alertable, zero timeout: STATUS_SUCCESS or STATUS_NO_YIELD_PERFORMED - Non-alertable, non-zero timeout: STATUS_SUCCESS - Alertable, zero timeout: STATUS_SUCCESS, STATUS_NO_YIELD_PERFORMED, or STATUS_USER_APC - Alertable, non-zero timeout: STATUS_SUCCESS or STATUS_USER_APC --- dlls/ntdll/tests/sync.c | 10 ++-------- dlls/ntdll/unix/server.c | 14 +++++++++++++- dlls/ntdll/unix/sync.c | 5 +++-- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 15f692e67bd..d308932078c 100644 --- a/dlls/ntdll/tests/sync.c +++ b/dlls/ntdll/tests/sync.c @@ -1078,14 +1078,12 @@ static DWORD WINAPI delay_thread( void *arg ) status = pNtDelayExecution( TRUE, &timeout ); if (!p->non_zero_timeout) { - todo_wine_if(status == STATUS_TIMEOUT) ok( status == STATUS_USER_APC || status == STATUS_NO_YIELD_PERFORMED || status == STATUS_SUCCESS, "NtDelayExecution iteration %d returned %#lx\n", i, status ); } else { - todo_wine_if(status == STATUS_TIMEOUT) ok( status == STATUS_USER_APC || status == STATUS_SUCCESS, "NtDelayExecution iteration %d with timeout returned %#lx\n", i, status ); @@ -1142,22 +1140,20 @@ static void test_delayexecution(void) } else if (!timeout.QuadPart) { - todo_wine_if(status == STATUS_TIMEOUT) ok( status == STATUS_SUCCESS || status == STATUS_NO_YIELD_PERFORMED || status == STATUS_USER_APC, "test %u: got %#lx, expected SUCCESS, NO_YIELD_PERFORMED, or USER_APC\n", j, status ); if (status == STATUS_NO_YIELD_PERFORMED) total_noyields[1]++; } else { - todo_wine_if(status == STATUS_TIMEOUT) ok( status == STATUS_SUCCESS || status == STATUS_USER_APC, "test %u: got %#lx, expected SUCCESS or USER_APC\n", j, status ); } } } - todo_wine ok( total_noyields[0] > 0, "Expected > 0 STATUS_NO_YIELD_PERFORMED results for non-alertable zero-timeout delays.\n" ); - todo_wine ok( total_noyields[1] > 0, "Expected > 0 STATUS_NO_YIELD_PERFORMED results for alertable zero-timeout delays.\n" ); + ok( total_noyields[0] > 0, "Expected > 0 STATUS_NO_YIELD_PERFORMED results for non-alertable zero-timeout delays.\n" ); + ok( total_noyields[1] > 0, "Expected > 0 STATUS_NO_YIELD_PERFORMED results for alertable zero-timeout delays.\n" ); param.ready = CreateEventA( NULL, FALSE, FALSE, NULL ); ok( param.ready != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); @@ -1182,10 +1178,8 @@ static void test_delayexecution(void) ret = WaitForSingleObject( param_timeout.ready, 5000 ); ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); - todo_wine ok( param.status == STATUS_SUCCESS || param.status == STATUS_NO_YIELD_PERFORMED, "thread NtDelayExecution returned %#lx\n", param.status ); - todo_wine ok( param_timeout.status == STATUS_SUCCESS || param_timeout.status == STATUS_NO_YIELD_PERFORMED, "thread NtDelayExecution (timeout) returned %#lx\n", param_timeout.status ); diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index b40e7c2a1d7..a568a79177e 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -784,7 +784,19 @@ unsigned int server_wait( const union select_op *select_op, data_size_t size, UI /* A test on Windows 2000 shows that Windows always yields during a wait, but a wait that is hit by an event gets a priority boost as well. This seems to model that behavior the closest. */ - if (ret == STATUS_TIMEOUT) NtYieldExecution(); + if (ret == STATUS_TIMEOUT) + { + int yield_ret = NtYieldExecution(); + /* The following should only be reached from NtDelayExecution. + It never returns STATUS_TIMEOUT directly, and propagates + the result of the yield for zero-timeout waits. + Otherwise, it reports STATUS_SUCCESS. */ + if (!select_op) + { + if (timeout && !timeout->QuadPart) ret = yield_ret; + else ret = STATUS_SUCCESS; + } + } return ret; } diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index d486b50001d..12f391ee6e0 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -1650,6 +1650,7 @@ NTSTATUS WINAPI NtDelayExecution( BOOLEAN alertable, const LARGE_INTEGER *timeou } else { + NTSTATUS ret; LARGE_INTEGER now; timeout_t when, diff; @@ -1660,8 +1661,8 @@ NTSTATUS WINAPI NtDelayExecution( BOOLEAN alertable, const LARGE_INTEGER *timeou } /* Note that we yield after establishing the desired timeout */ - NtYieldExecution(); - if (!when) return STATUS_SUCCESS; + ret = NtYieldExecution(); + if (!when) return ret; for (;;) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7169
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=150971 Your paranoid android. === w10pro64_ar (64 bit report) === ntdll: sync.c:1155: Test failed: Expected > 0 STATUS_NO_YIELD_PERFORMED results for non-alertable zero-timeout delays. sync.c:1156: Test failed: Expected > 0 STATUS_NO_YIELD_PERFORMED results for alertable zero-timeout delays. === w10pro64_ja (64 bit report) === ntdll: sync.c:1155: Test failed: Expected > 0 STATUS_NO_YIELD_PERFORMED results for non-alertable zero-timeout delays. === debian11b (64 bit WoW report) === user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000032900F4, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
William Horvath (@whrvth) commented about dlls/ntdll/unix/server.c:
/* A test on Windows 2000 shows that Windows always yields during a wait, but a wait that is hit by an event gets a priority boost as well. This seems to model that behavior the closest. */ - if (ret == STATUS_TIMEOUT) NtYieldExecution(); + if (ret == STATUS_TIMEOUT) + { + int yield_ret = NtYieldExecution(); + /* The following should only be reached from NtDelayExecution. + It never returns STATUS_TIMEOUT directly, and propagates + the result of the yield for zero-timeout waits. + Otherwise, it reports STATUS_SUCCESS. */ + if (!select_op)
I assume the main problem with this patch is due to this check here. While this works correctly for now with how `server_wait` is used, it's just an extra thing to be careful about in the future. However, the other options I considered also had tradeoffs: - Add an argument to `server_wait` to signal if we want the return value of the yield (best alternative) - Just yield again in `NtDelayExecution` itself, ignoring the fact that we already yielded, and return the result there like we do for the non-alertable case (second best alternative) - Move the yield outside of `server_wait` and duplicate it everywhere `server_wait` is called (terrible) Maybe there's something else to consider, but the current version isn't so bad if you take these alternatives into account. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92746
On Fri Jan 24 19:19:47 2025 +0000, William Horvath wrote:
I assume the main problem with this patch is due to this check here. While this works correctly for now with how `server_wait` is used, it's just an extra thing to be careful about in the future. However, the other options I considered also had tradeoffs: - Add an argument to `server_wait` to signal if we want the return value of the yield (best alternative) - Just yield again in `NtDelayExecution` itself, ignoring the fact that we already yielded, and return the result there like we do for the non-alertable case (second best alternative) - Move the yield outside of `server_wait` and duplicate it everywhere `server_wait` is called (terrible) Maybe there's something else to consider, but the current version isn't so bad if you take these alternatives into account. Detecting special case for NtDelayExecution based on select_op in server_select() is not particularly nice, maybe would be better to return yield status from server_wait() and handle everything in NtDelayExecution?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92747
On Fri Jan 24 19:38:58 2025 +0000, Paul Gofman wrote:
Detecting special case for NtDelayExecution based on select_op in server_select() is not particularly nice, maybe would be better to return yield status from server_wait() and handle everything in NtDelayExecution? I mean. return in a new output parameter, not as the function return value of course.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92748
On Fri Jan 24 19:41:04 2025 +0000, Paul Gofman wrote:
I mean. return in a new output parameter, not as the function return value of course. This is basically the "first alternative" that I suggested, but returning in the parameter instead of the return value. I can do that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92749
On Fri Jan 24 19:43:45 2025 +0000, William Horvath wrote:
This is basically the "first alternative" that I suggested, but returning in the parameter instead of the return value. I can do that. On a separate note, I'd also test GetLastError() after Sleep / SleepEx (doing SetLastError(0xdeadbeef) before each of test) and SleepEx return value, to see that it is all handled correctly with new NtDelayExecution returns. If there are any apps depending on proper "no yield" status and if GetLastError() reflects that I'd expect that to be used more often than calling Nt function directly.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92751
On Fri Jan 24 19:51:58 2025 +0000, Paul Gofman wrote:
On a separate note, I'd also test GetLastError() after Sleep / SleepEx (doing SetLastError(0xdeadbeef) before each of test) and SleepEx return value, to see that it is all handled correctly with new NtDelayExecution returns. If there are any apps depending on proper "no yield" status and if GetLastError() reflects that I'd expect that to be used more often than calling Nt function directly. I had a standalone test for this actually, and the patch fixed the Sleep/SleepEx GLE in the same way. I can add tests for these too, but where should I add them? A separate test for Sleep(Ex) in `dlls/kernelbase/tests/sync.c`, or just as an addition to the existing NtDelayExecution test?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92752
On Fri Jan 24 19:55:53 2025 +0000, William Horvath wrote:
I had a standalone test for this actually, and the patch fixed the Sleep/SleepEx GLE in the same way. I can add tests for these too, but where should I add them? A separate test for Sleep(Ex) in `dlls/kernelbase/tests/sync.c`, or just as an addition to the existing NtDelayExecution test? I am not entirely sure, but I think it may be done right in the same ntdll test. While doesn't belong exactly to ntdll, it seems easier to follow when closely related functionality has a test in the same place, and avoid repeating all the same not exactly one-liner test elsewhere.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92754
participants (4)
-
Marvin -
Paul Gofman (＠gofman) -
William Horvath
-
William Horvath (＠whrvth)