[PATCH v10 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 -- v10: 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 | 126 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 3ab89bbf361..8522a4f5966 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,129 @@ 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) + 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 ); + + 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) + 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].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() ); + } + + /* break out early once we see the expected result */ + if (!tests[i].alertable && !tests[i].timeout && noyields[0]) + break; +#if 0 /* TODO */ + if (tests[i].alertable && !tests[i].timeout && apc_status_count[0] && noyields[1]) + break; +#else + if (tests[i].alertable && !tests[i].timeout && apc_status_count[0] && status == STATUS_SUCCESS) + break; +#endif + if (tests[i].alertable && tests[i].timeout && apc_status_count[1]) + break; + } + 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 +1170,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 +1203,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 8522a4f5966..83d53ddbd05 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), @@ -1148,7 +1147,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..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
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=151073 Your paranoid android. === debian11b (64 bit WoW report) === user32: input.c:4306: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000012B00F0, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
On Mon Jan 27 16:10:58 2025 +0000, Paul Gofman wrote:
Sorry, 1600ms. But even 160ms of total test run time would be way too expensive. I hope the way I've done it in the latest version makes sense to you.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92945
On Mon Jan 27 16:31:44 2025 +0000, William Horvath wrote:
I hope the way I've done it in the latest version makes sense to you. Actually, it's broken, I forgot some cases to break out from.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92948
Paul Gofman (@gofman) commented about dlls/ntdll/tests/sync.c:
+ + 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() ); + } + + /* break out early once we see the expected result */ + if (!tests[i].alertable && !tests[i].timeout && noyields[0]) + break; +#if 0 /* TODO */ We place things which are to be skipped by stay for documentation in 'if (0)', not #ifdef. But I still think it is not needed and it is unclear what is TODO there, I think you might just remove those iterations, count arrays and allow both results for random part (_SUCCESS vs _NO_YIELD_PERFORMED).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92950
On Mon Jan 27 16:39:03 2025 +0000, Paul Gofman wrote:
We place things which are to be skipped by stay for documentation in 'if (0)', not #ifdef. But I still think it is not needed and it is unclear what is TODO there, I think you might just remove those iterations, count arrays and allow both results for random part (_SUCCESS vs _NO_YIELD_PERFORMED). In that case, what if I just lower the iteration count for any test case to 20, and don't break early at all? Then it simplifies the test to just making sure we don't see STATUS_TIMEOUT.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92954
On Mon Jan 27 16:51:37 2025 +0000, William Horvath wrote:
In that case, what if I just lower the iteration count for any test case to 20, and don't break early at all? Then it simplifies the test to just making sure we don't see STATUS_TIMEOUT. One iteration (that is, no iterations) is enough to see it. It is still flaky if you wait for yield to happen or not during given number of iterations. Making it this way with iterations doesn't make test more clear, but I'd personally wouldn't care too much if that wasn't flaky additionally.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92955
participants (4)
-
Marvin -
Paul Gofman (＠gofman) -
William Horvath
-
William Horvath (＠whrvth)