[PATCH v2 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 -- v2: 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 | 308 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 308 insertions(+) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index 3ab89bbf361..da070329d26 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,311 @@ 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 + */ + +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 ); + } + } + + 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 DWORD expected_sleep_error = 0xdeadbeef; + +static DWORD WINAPI sleep_thread( void *arg ) +{ + struct test_delay_param *p = arg; + DWORD ret, gle; + int i; + + SetEvent( p->ready ); + ret = WaitForSingleObject( p->completion, INFINITE ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + + for (i = 0; i < NUM_ITERATIONS; i++) + { + SetLastError( expected_sleep_error ); + if (p->non_zero_timeout) + ret = SleepEx( 1, TRUE ); + else + ret = SleepEx( 0, TRUE ); + + if (!p->non_zero_timeout) + { + gle = GetLastError(); + ok( gle == expected_sleep_error, "SleepEx iteration %d set error %lu, expected %lu\n", + i, gle, expected_sleep_error ); + ok( ret == 0 || ret == WAIT_IO_COMPLETION, + "SleepEx iteration %d returned %lu\n", i, ret ); + } + else + { + gle = GetLastError(); + ok( gle == expected_sleep_error, "SleepEx iteration %d with timeout set error %lu, expected %lu\n", + i, gle, expected_sleep_error ); + ok( ret == 0 || ret == WAIT_IO_COMPLETION, + "SleepEx iteration %d with timeout returned %lu\n", i, ret ); + } + } + + 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; + enum { API_NTDELAY, API_SLEEPEX, API_SLEEP } api; + } + tests[] = + { + { 0, FALSE, API_NTDELAY }, + { 0, TRUE, API_NTDELAY }, + { 1 * -10000, FALSE, API_NTDELAY }, + { 1 * -10000, TRUE, API_NTDELAY }, + { 10 * -10000, TRUE, API_NTDELAY }, + { 0, FALSE, API_SLEEPEX }, + { 0, TRUE, API_SLEEPEX }, + { 1 * -10000, FALSE, API_SLEEPEX }, + { 1 * -10000, TRUE, API_SLEEPEX }, + { 10 * -10000, TRUE, API_SLEEPEX }, + { 0, FALSE, API_SLEEP }, + { 1 * -10000, FALSE, API_SLEEP }, + }; + + struct test_delay_param param = {0}; + struct test_delay_param param_timeout = {0}; + struct test_delay_param sleep_param = {0}; + struct test_delay_param sleep_param_timeout = {0}; + unsigned int total_noyields[2] = {0, 0}; + LARGE_INTEGER timeout; + HANDLE thread, thread_timeout; + HANDLE sleep_thread_handle, sleep_thread_timeout; + NTSTATUS status; + DWORD tid, tid_timeout, sleep_tid, sleep_tid_timeout, ret, gle; + unsigned int i, j; + + for (i = 0; i < NUM_ITERATIONS; i++) + { + for (j = 0; j < ARRAY_SIZE(tests); j++) + { + timeout.QuadPart = tests[j].timeout; + SetLastError( expected_sleep_error ); + + switch (tests[j].api) + { + case API_NTDELAY: + 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 ); + } + break; + + case API_SLEEPEX: + ret = SleepEx( timeout.QuadPart ? (-timeout.QuadPart / 10000) : 0, tests[j].alertable ); + gle = GetLastError(); + ok( gle == expected_sleep_error, "test %u: SleepEx set error %lu, expected %lu\n", j, gle, expected_sleep_error ); + + if (!tests[j].alertable) + { + ok( ret == 0, "test %u: SleepEx returned %lu, expected 0\n", j, ret ); + } + else + { + ok( ret == 0 || ret == WAIT_IO_COMPLETION, + "test %u: SleepEx returned %lu, expected 0 or WAIT_IO_COMPLETION\n", j, ret ); + } + break; + + case API_SLEEP: + Sleep( timeout.QuadPart ? (-timeout.QuadPart / 10000) : 0 ); + gle = GetLastError(); + ok( gle == expected_sleep_error, "test %u: Sleep set error %lu, expected %lu\n", j, gle, expected_sleep_error ); + break; + } + } + } + + 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 ); + + /* Test Sleep/SleepEx behavior */ + sleep_param.ready = CreateEventA( NULL, FALSE, FALSE, NULL ); + ok( sleep_param.ready != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); + sleep_param.completion = CreateEventA( NULL, FALSE, FALSE, NULL ); + ok( sleep_param.completion != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); + sleep_param.non_zero_timeout = FALSE; + + sleep_param_timeout = sleep_param; + sleep_param_timeout.ready = CreateEventA( NULL, FALSE, FALSE, NULL ); + ok( sleep_param_timeout.ready != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); + sleep_param_timeout.completion = CreateEventA( NULL, FALSE, FALSE, NULL ); + ok( sleep_param_timeout.completion != NULL, "CreateEvent failed, error %lu\n", GetLastError() ); + sleep_param_timeout.non_zero_timeout = TRUE; + + sleep_thread_handle = CreateThread( NULL, 0, sleep_thread, &sleep_param, 0, &sleep_tid ); + ok( sleep_thread_handle != NULL, "CreateThread failed, error %lu\n", GetLastError() ); + sleep_thread_timeout = CreateThread( NULL, 0, sleep_thread, &sleep_param_timeout, 0, &sleep_tid_timeout ); + ok( sleep_thread_timeout != NULL, "CreateThread failed, error %lu\n", GetLastError() ); + + ret = WaitForSingleObject( sleep_param.ready, 5000 ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + ret = WaitForSingleObject( sleep_param_timeout.ready, 5000 ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + + 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() ); + ok( QueueUserAPC( apc_func, sleep_thread_handle, (ULONG_PTR)&sleep_param ), + "QueueUserAPC failed for zero timeout sleep thread, error %lu\n", GetLastError() ); + ok( QueueUserAPC( apc_func, sleep_thread_timeout, (ULONG_PTR)&sleep_param_timeout ), + "QueueUserAPC failed for non-zero timeout sleep thread, error %lu\n", GetLastError() ); + } + + SetEvent( param.completion ); + SetEvent( param_timeout.completion ); + SetEvent( sleep_param.completion ); + SetEvent( sleep_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 ); + ret = WaitForSingleObject( sleep_thread_handle, 5000 ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + ret = WaitForSingleObject( sleep_thread_timeout, 5000 ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject failed with %#lx\n", ret ); + + CloseHandle( thread ); + CloseHandle( thread_timeout ); + CloseHandle( sleep_thread_handle ); + CloseHandle( sleep_thread_timeout ); + CloseHandle( param.ready ); + CloseHandle( param.completion ); + CloseHandle( param_timeout.ready ); + CloseHandle( param_timeout.completion ); + CloseHandle( sleep_param.ready ); + CloseHandle( sleep_param.completion ); + CloseHandle( sleep_param_timeout.ready ); + CloseHandle( sleep_param_timeout.completion ); +} + START_TEST(sync) { HMODULE module = GetModuleHandleA("ntdll.dll"); @@ -1046,6 +1352,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 +1385,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 | 10 ++-------- dlls/ntdll/unix/server.c | 4 ++-- dlls/ntdll/unix/sync.c | 22 +++++++++++++++------- dlls/ntdll/unix/unix_private.h | 2 +- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/dlls/ntdll/tests/sync.c b/dlls/ntdll/tests/sync.c index da070329d26..e4e89a08cc3 100644 --- a/dlls/ntdll/tests/sync.c +++ b/dlls/ntdll/tests/sync.c @@ -1079,14 +1079,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 ); @@ -1201,14 +1199,12 @@ 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 ); } @@ -1239,8 +1235,8 @@ static void test_delayexecution(void) } } - 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() ); @@ -1265,10 +1261,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..3d173a3e4e0 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -764,7 +764,7 @@ unsigned int server_select( const union select_op *select_op, data_size_t size, * server_wait */ unsigned int server_wait( const union select_op *select_op, data_size_t size, UINT flags, - const LARGE_INTEGER *timeout ) + const LARGE_INTEGER *timeout, NTSTATUS *yield_result ) { timeout_t abs_timeout = timeout ? timeout->QuadPart : TIMEOUT_INFINITE; unsigned int ret; @@ -784,7 +784,7 @@ 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) (yield_result ? *yield_result = NtYieldExecution() : NtYieldExecution()); return ret; } diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 0ee16b360b3..54d36fc47d7 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -1580,7 +1580,7 @@ NTSTATUS WINAPI NtWaitForMultipleObjects( DWORD count, const HANDLE *handles, BO if (alertable) flags |= SELECT_ALERTABLE; select_op.wait.op = wait_any ? SELECT_WAIT : SELECT_WAIT_ALL; for (i = 0; i < count; i++) select_op.wait.handles[i] = wine_server_obj_handle( handles[i] ); - return server_wait( &select_op, offsetof( union select_op, wait.handles[count] ), flags, timeout ); + return server_wait( &select_op, offsetof( union select_op, wait.handles[count] ), flags, timeout, NULL ); } @@ -1608,7 +1608,7 @@ NTSTATUS WINAPI NtSignalAndWaitForSingleObject( HANDLE signal, HANDLE wait, select_op.signal_and_wait.op = SELECT_SIGNAL_AND_WAIT; select_op.signal_and_wait.wait = wine_server_obj_handle( wait ); select_op.signal_and_wait.signal = wine_server_obj_handle( signal ); - return server_wait( &select_op, sizeof(select_op.signal_and_wait), flags, timeout ); + return server_wait( &select_op, sizeof(select_op.signal_and_wait), flags, timeout, NULL ); } @@ -1641,8 +1641,16 @@ 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) + { + server_wait( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, timeout, &yield_result ); + /* we only care about the return value of the yield for zero timeouts */ + if (timeout && !timeout->QuadPart) return yield_result; + return STATUS_SUCCESS; + } if (!timeout || timeout->QuadPart == TIMEOUT_INFINITE) /* sleep forever */ { @@ -1660,8 +1668,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; + yield_result = NtYieldExecution(); + if (!when) return yield_result; for (;;) { @@ -1892,7 +1900,7 @@ NTSTATUS WINAPI NtWaitForKeyedEvent( HANDLE handle, const void *key, select_op.keyed_event.op = SELECT_KEYED_EVENT_WAIT; select_op.keyed_event.handle = wine_server_obj_handle( handle ); select_op.keyed_event.key = wine_server_client_ptr( key ); - return server_wait( &select_op, sizeof(select_op.keyed_event), flags, timeout ); + return server_wait( &select_op, sizeof(select_op.keyed_event), flags, timeout, NULL ); } @@ -1911,7 +1919,7 @@ NTSTATUS WINAPI NtReleaseKeyedEvent( HANDLE handle, const void *key, select_op.keyed_event.op = SELECT_KEYED_EVENT_RELEASE; select_op.keyed_event.handle = wine_server_obj_handle( handle ); select_op.keyed_event.key = wine_server_client_ptr( key ); - return server_wait( &select_op, sizeof(select_op.keyed_event), flags, timeout ); + return server_wait( &select_op, sizeof(select_op.keyed_event), flags, timeout, NULL ); } diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index f840045f841..8d9e6ec92f8 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -211,7 +211,7 @@ extern void server_leave_uninterrupted_section( pthread_mutex_t *mutex, sigset_t extern unsigned int server_select( const union select_op *select_op, data_size_t size, UINT flags, timeout_t abs_timeout, struct context_data *context, struct user_apc *user_apc ); extern unsigned int server_wait( const union select_op *select_op, data_size_t size, UINT flags, - const LARGE_INTEGER *timeout ); + const LARGE_INTEGER *timeout, NTSTATUS *yield_result ); extern unsigned int server_queue_process_apc( HANDLE process, const union apc_call *call, union apc_result *result ); extern int server_get_unix_fd( HANDLE handle, unsigned int wanted_access, int *unix_fd, -- 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=151043 Your paranoid android. === debian11 (32 bit report) === Report validation errors: ntdll:file crashed (c0000028) === debian11b (64 bit WoW report) === kernel32: comm.c:1586: Test failed: Unexpected time 1000, expected around 500 Report validation errors: kernel32:file crashed (80000101) ntdll:file crashed (80000101)
v2: - Add tests for Sleep(Ex). - Move the yield return to a parameter in `server_wait` and the return logic to `NtDelayExecution` itself. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92774
Paul Gofman (@gofman) 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) (yield_result ? *yield_result = NtYieldExecution() : NtYieldExecution());
This is not an entirely standard use of ternary operator, probably better to have something like ``` unsigned int ret, status; ... if (ret == STATUS_TIMEOUT) { status = NtYieldExecution(); if (yield_result) *yield_result = status; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92778
Paul Gofman (@gofman) commented about dlls/ntdll/tests/sync.c:
+ { + todo_wine_if(status == STATUS_TIMEOUT) + ok( status == STATUS_USER_APC || status == STATUS_SUCCESS, + "NtDelayExecution iteration %d with timeout returned %#lx\n", + i, status ); + } + } + + 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 DWORD expected_sleep_error = 0xdeadbeef; A variable for this is not needed, we use just 0xdeadbeef directly universally in tests.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92779
Paul Gofman (@gofman) commented about dlls/ntdll/tests/sync.c:
+ + 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; + enum { API_NTDELAY, API_SLEEPEX, API_SLEEP } api; + } Is coding function really necessary in the table? Probably less code and more readable to just call all the three inside the test loop body (yes, for Sleep() there will be some repeated tests in this case, so what)? Then maybe some of the branched checks there can be coalesced.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92780
Paul Gofman (@gofman) commented about dlls/ntdll/unix/server.c:
* server_wait */ unsigned int server_wait( const union select_op *select_op, data_size_t size, UINT flags, - const LARGE_INTEGER *timeout ) + const LARGE_INTEGER *timeout, NTSTATUS *yield_result ) ntdll Unix-side code now uses 'unsigned int' for internal NTSTATUS representation, probably best to stick to it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92777
Paul Gofman (@gofman) commented about dlls/ntdll/tests/sync.c:
+ break; + } + } + } + + 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 ); Maybe I am missing something, but why do you need those additional threads, is there any cross-thread specific here? You could maybe just queue APCs to the same thread and check satisfied and timed out waits also on the same thread setting the desired event state upfront?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7169#note_92781
participants (4)
-
Marvin -
Paul Gofman (＠gofman) -
William Horvath
-
William Horvath (＠whrvth)