[PATCH v4 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 -- v4: 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 | 114 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 3ab89bbf361..813d72ca6a0 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,117 @@ 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 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" }, +}; + +static void test_delayexecution(void) +{ + unsigned int i, iterations; + unsigned int noyields[2] = {0, 0}; /* [0] = non-alertable, [1] = alertable */ + unsigned int apc_status_count[2] = {0, 0}; /* [0] = zero timeout, [1] = non-zero timeout */ + 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); + + /* run zero-timeout tests a lot more to catch possibly flaky (but very unlikely) NO_YIELD_PERFORMED */ + iterations = tests[i].timeout ? 100 : 10000; + while (iterations--) + { + timeout.QuadPart = tests[i].timeout; + + apc_count = 0; + if (tests[i].queue_apc) + ok( QueueUserAPC( apc_proc, GetCurrentThread(), (ULONG_PTR)&apc_count ), "failed to queue APC.\n" ); + + /* 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 ); + + if (status == STATUS_NO_YIELD_PERFORMED) + noyields[tests[i].alertable]++; + + /* make sure we only get APCs when we expect them */ + if (tests[i].alertable && tests[i].queue_apc) + { + if (status == STATUS_USER_APC) + apc_status_count[tests[i].timeout ? 1 : 0]++; + ok( apc_count > 0, "no APCs executed.\n" ); + } + else + ok( !apc_count, "unexpected APCs executed %ld.\n", apc_count ); + + /* test SleepEx */ + apc_count = 0; + if (tests[i].queue_apc) + ok( QueueUserAPC( apc_proc, GetCurrentThread(), (ULONG_PTR)&apc_count ), "failed to queue APC.\n" ); + + 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].queue_apc) + ok( apc_count > 0, "no APCs executed.\n" ); + + /* 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(); + } + + todo_wine ok( noyields[0] > 0, "no STATUS_NO_YIELD_PERFORMED results for non-alertable yields.\n" ); + todo_wine ok( noyields[1] > 0, "no STATUS_NO_YIELD_PERFORMED results for alertable yields.\n" ); + ok( apc_status_count[0] > 0, "no STATUS_USER_APC results for alertable yields.\n" ); + ok( apc_status_count[1] > 0, "no STATUS_USER_APC results for alertable delays.\n" ); +} + START_TEST(sync) { HMODULE module = GetModuleHandleA("ntdll.dll"); @@ -1046,6 +1158,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 +1191,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 | 3 +-- dlls/ntdll/unix/sync.c | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 813d72ca6a0..49305911256 100644 --- a/dlls/ntdll/tests/sync.c +++ b/dlls/ntdll/tests/sync.c @@ -1089,7 +1089,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), @@ -1136,7 +1135,7 @@ static void test_delayexecution(void) winetest_pop_context(); } - todo_wine ok( noyields[0] > 0, "no STATUS_NO_YIELD_PERFORMED results for non-alertable yields.\n" ); + ok( noyields[0] > 0, "no STATUS_NO_YIELD_PERFORMED results for non-alertable yields.\n" ); todo_wine ok( noyields[1] > 0, "no STATUS_NO_YIELD_PERFORMED results for alertable yields.\n" ); ok( apc_status_count[0] > 0, "no STATUS_USER_APC results for alertable yields.\n" ); ok( apc_status_count[1] > 0, "no STATUS_USER_APC results for alertable delays.\n" ); diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 0ee16b360b3..6e82e308a66 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 ) { + NTSTATUS yield_result = 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 (server_wait( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, timeout ) == STATUS_USER_APC) + yield_result = STATUS_USER_APC; + return yield_result; + } 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 */ + yield_result = NtYieldExecution(); + if (!when) return yield_result; for (;;) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7169
v3: - Remove returning "fake" `NtYieldExecution()` result for alertable waits, and instead always return `_SUCCESS` or `_USER_APC`, as we always implicitly yield going through `server_select` anyways - Move struct inside test body - Use `winetest_push/pop_context` - Swap inner/outer test arrays, and test zero timeout waits a lot more - Clean up `ok()` prints - Remove unnecessary multiple queued APCs - Consolidate unnecessary if/else - Apply other style suggestions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92838
On Sat Jan 25 06:52:43 2025 +0000, William Horvath wrote:
v3: - Remove returning "fake" `NtYieldExecution()` result for alertable waits, and instead always return `_SUCCESS` or `_USER_APC`, as we always implicitly yield going through `server_select` anyways - Move struct inside test body - Use `winetest_push/pop_context` - Swap inner/outer test arrays, and test zero timeout waits a lot more - Clean up `ok()` prints - Remove unnecessary multiple queued APCs - Consolidate unnecessary if/else - Apply other style suggestions. Ah, somehow I missed the struct->inside body in this version...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92840
participants (2)
-
William Horvath -
William Horvath (＠whrvth)