[PATCH v6 0/2] MR7169: ntdll: Fix the return value of NtDelayExecution.

[William Horvath (＠whrvth)]

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

--
  v6: 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

[PATCH v6 1/2] ntdll/tests: Add tests for NtDelayExecution and Sleep(Ex).

[William Horvath]

From: William Horvath

---
 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..fa73ef37d8c 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]++;
+                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();
 }

[PATCH v6 2/2] ntdll: Fix the return value of NtDelayExecution.

[William Horvath]

From: William Horvath

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 fa73ef37d8c..4fd657b4c0f 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 (;;)
         {

[PATCH v6 2/2] ntdll: Fix the return value of NtDelayExecution.

[Marvin]

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=151057

Your paranoid android.

=== w7u_2qxl (32 bit report) ===

ntdll:
0548:sync: unhandled exception c0000005 at 001A555F

=== w7u_adm (32 bit report) ===

ntdll:
0790:sync: unhandled exception c0000005 at 00F7555F

=== w7u_el (32 bit report) ===

ntdll:
0a64:sync: unhandled exception c0000005 at 0036555F

=== w8 (32 bit report) ===

ntdll:
0c08:sync: unhandled exception c0000005 at 0083555F

=== w8adm (32 bit report) ===

ntdll:
0b20:sync: unhandled exception c0000005 at 0027555F

=== w864 (32 bit report) ===

ntdll:
04fc:sync: unhandled exception c0000005 at 00EF555F

=== w1064v1507 (32 bit report) ===

ntdll:
0b8c:sync: unhandled exception c0000005 at 0130555F

=== w1064v1809 (32 bit report) ===

ntdll:
1f2c:sync: unhandled exception c0000005 at 0126555F

=== w1064_tsign (32 bit report) ===

ntdll:
1f20:sync: unhandled exception c0000005 at 008D555F

=== w10pro64 (32 bit report) ===

ntdll:
20f0:sync: unhandled exception c0000005 at 0081555F

=== w10pro64_en_AE_u8 (32 bit report) ===

ntdll:
22c4:sync: unhandled exception c0000005 at 002F555F

=== w11pro64 (32 bit report) ===

ntdll:
1ee4:sync: unhandled exception c0000005 at 00C4555F

=== w7pro64 (64 bit report) ===

ntdll:
0604:sync: unhandled exception c0000005 at 0000000000F716AB

=== w864 (64 bit report) ===

ntdll:
0128:sync: unhandled exception c0000005 at 00000000009F16AB

=== w1064v1507 (64 bit report) ===

ntdll:
0cf4:sync: unhandled exception c0000005 at 0000000000EF16AB

=== w1064v1809 (64 bit report) ===

ntdll:
1e64:sync: unhandled exception c0000005 at 0000000000C716AB

=== w1064_2qxl (64 bit report) ===

ntdll:
1dc4:sync: unhandled exception c0000005 at 00000000005016AB

=== w1064_adm (64 bit report) ===

ntdll:
1e14:sync: unhandled exception c0000005 at 00000000008616AB

=== w1064_tsign (64 bit report) ===

ntdll:
1df0:sync: unhandled exception c0000005 at 0000000000F316AB

=== w10pro64 (64 bit report) ===

ntdll:
224c:sync: unhandled exception c0000005 at 0000000000B016AB

=== w10pro64_ar (64 bit report) ===

ntdll:
1fa8:sync: unhandled exception c0000005 at 0000000000C216AB

=== w10pro64_ja (64 bit report) ===

ntdll:
1a2c:sync: unhandled exception c0000005 at 00000000009D16AB

=== w10pro64_zh_CN (64 bit report) ===

ntdll:
1c10:sync: unhandled exception c0000005 at 0000000000AB16AB

=== w11pro64_amd (64 bit report) ===

ntdll:
23fc:sync: unhandled exception c0000005 at 00000000001D16AB

=== debian11 (32 bit report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11 (32 bit ar:MA report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11 (32 bit de report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11 (32 bit fr report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11 (32 bit he:IL report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11 (32 bit hi:IN report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11 (32 bit ja:JP report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11 (32 bit zh:CN report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11b (32 bit WoW report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

=== debian11b (64 bit WoW report) ===

ntdll:
sync.c:1140: Test failed: no STATUS_USER_APC results for alertable delays.

user32:
input.c:4306: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 0000000000A300F6, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032