[PATCH v11 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 -- v11: ntdll: Fix the return value of NtDelayExecution. ntdll/tests: Add tests for NtDelayExecution and Sleep(Ex). https://gitlab.winehq.org/wine/wine/-/merge_requests/7169
From: William Horvath <william(a)horvath.blog> --- dlls/ntdll/tests/sync.c | 92 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 3ab89bbf361..d8f5191f6a2 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,95 @@ 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 + * - Sleep/SleepEx don't modify LastError, no matter what + */ + +static VOID CALLBACK apc_proc( ULONG_PTR param ) +{ + InterlockedIncrement( (LONG *)param ); +} + +static void test_delayexecution(void) +{ + static const struct + { + BOOLEAN alertable; + LONGLONG timeout; + BOOLEAN queue_apc; + const char *desc; + } tests[] = + { + { FALSE, 0, FALSE, "non-alertable yield" }, + { FALSE, -10000, FALSE, "non-alertable sleep" }, + { TRUE, 0, FALSE, "alertable yield" }, + { TRUE, -10000, FALSE, "alertable sleep" }, + { TRUE, 0, TRUE, "alertable yield with APC" }, + { TRUE, -10000, TRUE, "alertable sleep with APC" }, + }; + unsigned int i; + LARGE_INTEGER timeout; + ULONG apc_count; + NTSTATUS status; + DWORD ret; + + for (i = 0; i < ARRAY_SIZE(tests); i++) + { + winetest_push_context("%s", tests[i].desc); + timeout.QuadPart = tests[i].timeout; + + apc_count = 0; + if (tests[i].queue_apc) + QueueUserAPC( apc_proc, GetCurrentThread(), (ULONG_PTR)&apc_count ); + + /* test NtDelayExecution */ + SetLastError( 0xdeadbeef ); + status = pNtDelayExecution( tests[i].alertable, &timeout ); + ok( GetLastError() == 0xdeadbeef, "got error %#lx.\n", GetLastError() ); + + todo_wine_if(status == STATUS_TIMEOUT); + ok( status == STATUS_SUCCESS || + (!tests[i].timeout && status == STATUS_NO_YIELD_PERFORMED) || + (tests[i].alertable && tests[i].queue_apc && status == STATUS_USER_APC), + "got status %#lx.\n", status ); + + /* make sure we only get APCs when we expect them */ + if (tests[i].alertable && !tests[i].queue_apc) + ok( !apc_count, "unexpected APCs executed %ld.\n", apc_count ); + + /* test SleepEx */ + apc_count = 0; + if (tests[i].queue_apc) + QueueUserAPC( apc_proc, GetCurrentThread(), (ULONG_PTR)&apc_count ); + + SetLastError( 0xdeadbeef ); + ret = SleepEx( timeout.QuadPart ? (-timeout.QuadPart / 10000) : 0, tests[i].alertable ); + ok( GetLastError() == 0xdeadbeef, "got error %#lx.\n", GetLastError() ); + + if (!tests[i].alertable) + ok( !ret, "got %lu.\n", ret ); + else + ok( !ret || ret == WAIT_IO_COMPLETION, "got %lu.\n", ret ); + + if (tests[i].alertable && !tests[i].queue_apc) + ok( !apc_count, "unexpected APCs executed %ld.\n", apc_count ); + + /* test Sleep (non-alertable only) */ + if (!tests[i].alertable) + { + SetLastError( 0xdeadbeef ); + Sleep( timeout.QuadPart ? (-timeout.QuadPart / 10000) : 0 ); + ok( GetLastError() == 0xdeadbeef, "got error %#lx.\n", GetLastError() ); + } + + winetest_pop_context(); + } +} + START_TEST(sync) { HMODULE module = GetModuleHandleA("ntdll.dll"); @@ -1046,6 +1136,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 +1169,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 - Sleep/SleepEx don't modify LastError, no matter what --- dlls/ntdll/tests/sync.c | 1 - dlls/ntdll/unix/sync.c | 18 ++++++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index d8f5191f6a2..3ccdbd5cfbc 100644 --- a/dlls/ntdll/tests/sync.c +++ b/dlls/ntdll/tests/sync.c @@ -1081,7 +1081,6 @@ static void test_delayexecution(void) status = pNtDelayExecution( tests[i].alertable, &timeout ); ok( GetLastError() == 0xdeadbeef, "got error %#lx.\n", GetLastError() ); - todo_wine_if(status == STATUS_TIMEOUT); ok( status == STATUS_SUCCESS || (!tests[i].timeout && status == STATUS_NO_YIELD_PERFORMED) || (tests[i].alertable && tests[i].queue_apc && status == STATUS_USER_APC), diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 0ee16b360b3..2b01aaf83b8 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -1641,8 +1641,17 @@ NTSTATUS WINAPI NtYieldExecution(void) */ NTSTATUS WINAPI NtDelayExecution( BOOLEAN alertable, const LARGE_INTEGER *timeout ) { + unsigned int status = STATUS_SUCCESS; + /* if alertable, we need to query the server */ - if (alertable) return server_wait( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, timeout ); + if (alertable) + { + /* Since server_wait will result in an unconditional implicit yield, + we never return STATUS_NO_YIELD_PERFORMED */ + if ((status = server_wait( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, timeout )) == STATUS_TIMEOUT) + status = STATUS_SUCCESS; + return status; + } if (!timeout || timeout->QuadPart == TIMEOUT_INFINITE) /* sleep forever */ { @@ -1659,9 +1668,10 @@ NTSTATUS WINAPI NtDelayExecution( BOOLEAN alertable, const LARGE_INTEGER *timeou when = now.QuadPart - when; } - /* Note that we yield after establishing the desired timeout */ - NtYieldExecution(); - if (!when) return STATUS_SUCCESS; + /* Note that we yield after establishing the desired timeout, but + we only care about the result of the yield for zero timeouts */ + status = NtYieldExecution(); + if (!when) return status; for (;;) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7169
On Mon Jan 27 17:07:00 2025 +0000, William Horvath wrote:
changed this line in [version 11 of the diff](/wine/wine/-/merge_requests/7169/diffs?diff_id=154256&start_sha=e261343a5bc02af3bc1896f2b5814fc0fd63337a#78064f886cf71bafdbe5ff1e7794d63c4bdda9bb_1137_1116) Removed the extra iterations, just testing for any status in the set of possible ones.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92959
Paul Gofman (@gofman) commented about dlls/ntdll/tests/sync.c:
+ + for (i = 0; i < ARRAY_SIZE(tests); i++) + { + winetest_push_context("%s", tests[i].desc); + timeout.QuadPart = tests[i].timeout; + + apc_count = 0; + if (tests[i].queue_apc) + QueueUserAPC( apc_proc, GetCurrentThread(), (ULONG_PTR)&apc_count ); + + /* test NtDelayExecution */ + SetLastError( 0xdeadbeef ); + status = pNtDelayExecution( tests[i].alertable, &timeout ); + ok( GetLastError() == 0xdeadbeef, "got error %#lx.\n", GetLastError() ); + + todo_wine_if(status == STATUS_TIMEOUT); There are test failures in Wine after the patch 1 (due to spurious semicolon ';' in the end of this line).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92960
Paul Gofman (@gofman) commented about dlls/ntdll/tests/sync.c:
+ timeout.QuadPart = tests[i].timeout; + + apc_count = 0; + if (tests[i].queue_apc) + QueueUserAPC( apc_proc, GetCurrentThread(), (ULONG_PTR)&apc_count ); + + /* test NtDelayExecution */ + SetLastError( 0xdeadbeef ); + status = pNtDelayExecution( tests[i].alertable, &timeout ); + ok( GetLastError() == 0xdeadbeef, "got error %#lx.\n", GetLastError() ); + + todo_wine_if(status == STATUS_TIMEOUT); + ok( status == STATUS_SUCCESS || + (!tests[i].timeout && status == STATUS_NO_YIELD_PERFORMED) || + (tests[i].alertable && tests[i].queue_apc && status == STATUS_USER_APC), + "got status %#lx.\n", status ); In fact, APC is always processed in alertable sleep if APC is available, it is not random. Not sure, maybe in early patches versions when another thread was involved there was some race. So there is no alternative here, if apc is queued and wait is alertable, APC should be processed and reflected in the status. Also, test currently has leftover queued APC. How about incorporating the attached diff (the diff is on top but would be applied to the first patch): it solves those (in particular, pops leftover APC with SleepEx(0, TRUE)) and makes this a bit more straightforward.
[test.patch](/uploads/a38e87b1795d7259321cf46b50808e1d/test.patch) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92961
On Mon Jan 27 17:37:54 2025 +0000, Paul Gofman wrote:
In fact, APC is always processed in alertable sleep if APC is available, it is not random. Not sure, maybe in early patches versions when another thread was involved there was some race. So there is no alternative here, if apc is queued and wait is alertable, APC should be processed and reflected in the status. Also, test currently has leftover queued APC. How about incorporating the attached diff (the diff is on top but would be applied to the first patch): it solves those (in particular, pops leftover APC with SleepEx(0, TRUE)) and makes this a bit more straightforward. [test.patch](/uploads/a38e87b1795d7259321cf46b50808e1d/test.patch) Yes, that was an issue in earlier versions. Your diff makes sense, but we don't have any test cases where `!alertable && queue_apc`, so the `else...if (tests[i].queue_apc)` branch will never run. It should do the work of popping leftover APCs if I just move those outside the `else`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92963
On Mon Jan 27 17:54:09 2025 +0000, William Horvath wrote:
Yes, that was an issue in earlier versions. Your diff makes sense, but we don't have any test cases where `!alertable && queue_apc`, so the `else...if (tests[i].queue_apc)` branch will never run. It should do the work of popping leftover APCs if I just move those outside the `else`. I think we have, tests 3, 4 counting from 1? If maybe those are not needed then queue_apc should just be removed from the test structure, otherwise it is cleaner to always make sure that there are no APCs queued after each test iteration.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92964
On Mon Jan 27 17:58:08 2025 +0000, Paul Gofman wrote:
I think we have, tests 3, 4 counting from 1? If maybe those are not needed then queue_apc should just be removed from the test structure, otherwise it is cleaner to always make sure that there are no APCs queued after each test iteration. Tests 3 and 4 are alertable without APCs queued, tests 5 and 6 are alertable with APCs queued. But adding the extra `SleepEx(0, TRUE)` if we queued an APC is harmless.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92965
On Mon Jan 27 18:03:22 2025 +0000, William Horvath wrote:
Tests 3 and 4 are alertable without APCs queued, tests 5 and 6 are alertable with APCs queued. But adding the extra `SleepEx(0, TRUE)` if we queued an APC is harmless. ``` if (tests[i].queue_apc) SleepEx( 0, TRUE ); ``` after the alertable NtDelay and SleepEx. Sorry, yes, we indeed don't have, but it is hard to follow that it never happens in the table. Maybe just leave SleepEx( 0, TRUE ); outside of any conditions and without any checks after that, to make it clear that we make sure there are no leftover APCs whatever the test configuration is.
UPDATED: edited. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92966
On Mon Jan 27 18:03:08 2025 +0000, Paul Gofman wrote:
Sorry, yes, we indeed don't have, but it is hard to follow that it never happens in the table. Maybe just leave SleepEx( 0, TRUE ); outside of any conditions and without any checks after that, to make it clear that we make sure there are no leftover APCs whatever the test configuration is. UPDATED: edited. yes, I was about to do that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92967
participants (3)
-
Paul Gofman (＠gofman) -
William Horvath
-
William Horvath (＠whrvth)