[PATCH v7 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 -- v7: 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 | 113 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 3ab89bbf361..ca8b81d79e0 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,116 @@ 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, 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 +1157,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 +1190,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 ca8b81d79e0..5a9561261df 100644 --- a/dlls/ntdll/tests/sync.c +++ b/dlls/ntdll/tests/sync.c @@ -1088,7 +1088,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), @@ -1135,7 +1134,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
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=151058 Your paranoid android. === w1064_2qxl (64 bit report) === ntdll: sync.c:1138: Test failed: no STATUS_NO_YIELD_PERFORMED results for alertable yields. === debian11b (64 bit WoW report) === user32: input.c:4306: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000020A00EC, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
Agh... sorry, I thought I might as well was "fix a minor oversight" since the flaky d3d10core.c tests are overflowing the results, and I have to re-run the tests to see them, but that was obviously just wrong. This version should be correct. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92845
participants (3)
-
Marvin -
William Horvath -
William Horvath (＠whrvth)