[PATCH v5 0/2] MR3504: ntdll: An implementation of SRWLOCK that closer matches Windows'.

[Yuxuan Shui (＠yshui)]

This comes from behavioral study of Windows, which doesn't seem to check if the
lock is actually exclusively held, and just simply decrement the value stored
in the lock.

This fixes a dead lock which prevents WeCom from starting up.

--
  v5: ntdll: An implementation of SRWLOCK that closer matches Windows'.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3504

[PATCH v5 1/2] include: add atomic read/write of pointers

[Yuxuan Shui]

From: Yuxuan Shui <yshui(a)codeweavers.com>

---
 include/winnt.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/winnt.h b/include/winnt.h
index c04f25b29bd..7ccbc130956 100644
--- a/include/winnt.h
+++ b/include/winnt.h
@@ -7123,6 +7123,13 @@ static FORCEINLINE LONG ReadAcquire( LONG const volatile *src )
     return value;
 }
 
+static FORCEINLINE void *ReadAcquirePointer( void *const volatile *src )
+{
+    void *value;
+    __WINE_ATOMIC_LOAD_ACQUIRE( src, &value );
+    return value;
+}
+
 static FORCEINLINE LONG ReadNoFence( LONG const volatile *src )
 {
     LONG value;
@@ -7140,6 +7147,11 @@ static FORCEINLINE void WriteNoFence( LONG volatile *dest, LONG value )
     __WINE_ATOMIC_STORE_RELAXED( dest, &value );
 }
 
+static FORCEINLINE void WriteReleasePointer( void *volatile *src, void *value )
+{
+    __WINE_ATOMIC_STORE_RELEASE( src, &value );
+}
+
 static FORCEINLINE DECLSPEC_NORETURN void __fastfail(unsigned int code)
 {
 #if defined(__x86_64__) || defined(__i386__)
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/3504

[PATCH v5 2/2] ntdll: An implementation of SRWLOCK that closer matches Windows'.

[Yuxuan Shui]

From: Yuxuan Shui <yshui(a)codeweavers.com>

---
 dlls/kernel32/tests/sync.c |  24 ++
 dlls/ntdll/sync.c          | 533 ++++++++++++++++++++++++++-----------
 2 files changed, 401 insertions(+), 156 deletions(-)

diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c
index 60180194b7a..4eaec87c52f 100644
--- a/dlls/kernel32/tests/sync.c
+++ b/dlls/kernel32/tests/sync.c
@@ -2533,6 +2533,29 @@ static void test_srwlock_example(void)
     trace("number of total exclusive accesses is %ld\n", srwlock_protected_value);
 }
 
+static void test_srwlock_quirk(void)
+{
+    union { SRWLOCK *s; LONG *l; } u = { &srwlock_example };
+
+    if (!pInitializeSRWLock) {
+        /* function is not yet in XP, only in newer Windows */
+        win_skip("no srw lock support.\n");
+        return;
+    }
+
+    *u.l = 0;
+    pReleaseSRWLockExclusive(&srwlock_example);
+    ok(*u.l == 0xffffffff, "expected 0xffffffff, got %lx\n", *u.l);
+
+    *u.l = 1;
+    pReleaseSRWLockExclusive(&srwlock_example);
+    ok(*u.l == 0, "expected 0x0, got %lx\n", *u.l);
+
+    *u.l = 0x10000;
+    pReleaseSRWLockExclusive(&srwlock_example);
+    ok(*u.l == 0xffff, "expected 0xffff, got %lx\n", *u.l);
+}
+
 static DWORD WINAPI alertable_wait_thread(void *param)
 {
     HANDLE *semaphores = param;
@@ -2887,6 +2910,7 @@ START_TEST(sync)
     test_srwlock_base(&aligned_srwlock);
     test_srwlock_base(&unaligned_srwlock.lock);
     test_srwlock_example();
+    test_srwlock_quirk();
     test_alertable_wait();
     test_apc_deadlock();
     test_crit_section();
diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c
index fa64917029a..b1cb1adb0fe 100644
--- a/dlls/ntdll/sync.c
+++ b/dlls/ntdll/sync.c
@@ -26,6 +26,8 @@
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdint.h>
+#include <assert.h>
 #include <time.h>
 
 #include "ntstatus.h"
@@ -471,24 +473,83 @@ DWORD WINAPI RtlRunOnceExecuteOnce( RTL_RUN_ONCE *once, PRTL_RUN_ONCE_INIT_FN fu
     return RtlRunOnceComplete( once, 0, context ? *context : NULL );
 }
 
-struct srw_lock
-{
-    short exclusive_waiters;
+/**
+ * The SRWLOCK is a tagged pointer, with the last 4 bits been the tag.
+ * The pointer part serves 2 roles: if there is no waiters on the lock,
+ * it counts the number of shared owners of the lock; otherwise, it
+ * points to the end of the waiter queue, which is a linked list of waiters.
+ * This also implies the pointer must be 16 byte aligned.
+ *
+ * There are a couple helper functions below to make accessing different parts
+ * of the SRWLOCK easier.
+ */
+
+enum srwlock_tag {
+    SRWLOCK_TAG_LOCKED = 1,
+    SRWLOCK_TAG_HAS_WAITERS = 2,
+    /* Unknown purpose on Windows, but we use it to indicate the list
+     * is being modified. */
+    SRWLOCK_TAG_LIST_LOCKED = 4,
+    /* Unclear purpose on Windows, might be indicating this lock has
+     * > 1 owners. */
+    SRWLOCK_TAG_HAS_MULTIPLE_OWNERS = 8,
+};
 
-    /* Number of shared owners, or -1 if owned exclusive.
-     *
-     * Sadly Windows has no equivalent to FUTEX_WAIT_BITSET, so in order to wake
-     * up *only* exclusive or *only* shared waiters (and thus avoid spurious
-     * wakeups), we need to wait on two different addresses.
-     * RtlAcquireSRWLockShared() needs to know the values of "exclusive_waiters"
-     * and "owners", but RtlAcquireSRWLockExclusive() only needs to know the
-     * value of "owners", so the former can wait on the entire structure, and
-     * the latter waits only on the "owners" member. Note then that "owners"
-     * must not be the first element in the structure.
+enum srwlock_waiter_state {
+    SRWLOCK_WAITER_STATE_IS_EXCLUSIVE = 1,
+    /* ???? = 2, */
+    SRWLOCK_WAITER_STATE_NOTIFIED = 4,
+};
+DECLSPEC_ALIGN(16) struct srwlock_waiter {
+    /* Note the prev pointer is uninitialized for the list head. */
+    struct srwlock_waiter *prev;
+    struct srwlock_waiter *head;
+    struct srwlock_waiter *next;
+    DWORD thread_id;
+#ifdef _WIN64
+    DWORD padding;
+#endif
+    /* When the first waiter appears, the shared owner counter store in
+     * SRWLOCK is transferred here, however only the lower 28 bits are kept.
+     * A special value 0xffffffff indicates this waiter is not at the
+     * head of the list, and 0xfffffffe indicates this waiter is at the head
+     * of the list, but the lock is held exclusively.
      */
-    short owners;
+    DWORD num_owners;
+
+    /* A bitflag of states, see `enum srwlock_waiter_state`. */
+    DWORD state;
 };
-C_ASSERT( sizeof(struct srw_lock) == 4 );
+
+const DWORD SRWLOCK_WAITER_IS_NOT_HEAD = 0xffffffff;
+const DWORD SRWLOCK_WAITER_EXCLUSIVELY_LOCKED = 0xfffffffe;
+
+static inline struct srwlock_waiter *srwlock_get_waiter(RTL_SRWLOCK lock) {
+    uintptr_t ptr = (uintptr_t)lock.Ptr;
+    return (void *)(ptr & ~(uintptr_t)0xf);
+}
+
+static inline size_t srwlock_get_shared_count(RTL_SRWLOCK lock) {
+    uintptr_t ptr = (uintptr_t)lock.Ptr;
+    return (size_t)((ptr & (uintptr_t)0xfffffff0) >> 4);
+}
+
+static inline USHORT srwlock_get_tag(RTL_SRWLOCK lock) {
+    uintptr_t ptr = (uintptr_t)lock.Ptr;
+    return (USHORT)(ptr & (uintptr_t)0xf);
+}
+
+static inline RTL_SRWLOCK srwlock_from_waiter(struct srwlock_waiter *waiter, USHORT tag) {
+    RTL_SRWLOCK lock;
+    lock.Ptr = (void *)((uintptr_t)waiter | (uintptr_t)tag);
+    return lock;
+}
+
+static inline RTL_SRWLOCK srwlock_from_shared_owner_count(size_t count, USHORT tag) {
+    RTL_SRWLOCK lock;
+    lock.Ptr = (void *)(((uintptr_t)count << 4) | (uintptr_t)tag);
+    return lock;
+}
 
 /***********************************************************************
  *              RtlInitializeSRWLock (NTDLL.@)
@@ -496,141 +557,205 @@ C_ASSERT( sizeof(struct srw_lock) == 4 );
  * NOTES
  *  Please note that SRWLocks do not keep track of the owner of a lock.
  *  It doesn't make any difference which thread for example unlocks an
- *  SRWLock (see corresponding tests). This implementation uses two
- *  keyed events (one for the exclusive waiters and one for the shared
- *  waiters) and is limited to 2^15-1 waiting threads.
+ *  SRWLock (see corresponding tests).
  */
 void WINAPI RtlInitializeSRWLock( RTL_SRWLOCK *lock )
 {
     lock->Ptr = NULL;
 }
 
-/***********************************************************************
- *              RtlAcquireSRWLockExclusive (NTDLL.@)
- *
- * NOTES
- *  Unlike RtlAcquireResourceExclusive this function doesn't allow
- *  nested calls from the same thread. "Upgrading" a shared access lock
- *  to an exclusive access lock also doesn't seem to be supported.
- */
-void WINAPI RtlAcquireSRWLockExclusive( RTL_SRWLOCK *lock )
+/* Transfer the ownership of the lock to the waiters at the front of the list,
+ * remove them from the list, and wake them up. Only one thread can call this
+ * function at any given time. */
+static void srwlock_transfer_ownership(RTL_SRWLOCK *lock)
 {
-    union { RTL_SRWLOCK *rtl; struct srw_lock *s; LONG *l; } u = { lock };
-
-    InterlockedIncrement16( &u.s->exclusive_waiters );
-
-    for (;;)
-    {
-        union { struct srw_lock s; LONG l; } old, new;
-        BOOL wait;
+    RTL_SRWLOCK old, new, read;
+    USHORT tag;
+    struct srwlock_waiter *last, *head, *last_up_to_date, *last_to_remove,
+                          *new_head;
+
+    TRACE("%p\n", lock);
+
+    do {
+        old.Ptr = ReadAcquirePointer(&lock->Ptr);
+        tag = srwlock_get_tag(old);
+        last = srwlock_get_waiter(old);
+
+        assert(!(tag & SRWLOCK_TAG_LIST_LOCKED));
+        tag |= SRWLOCK_TAG_LIST_LOCKED;
+        new = srwlock_from_waiter(last, tag);
+    } while (InterlockedCompareExchangePointer(&lock->Ptr, new.Ptr, old.Ptr) !=
+             old.Ptr);
+
+    assert(tag & SRWLOCK_TAG_HAS_WAITERS);
+    old = new;
+    last = srwlock_get_waiter(old);
+    assert(last != NULL);
+    head = last->head;
+    assert(head != NULL);
+    last_up_to_date = last; /* the last waiter whose head pointer is up-to-date. */
+    new_head = NULL;
+    assert(head->num_owners == 0 ||
+           head->num_owners == SRWLOCK_WAITER_EXCLUSIVELY_LOCKED);
+    head->num_owners = 0;
+    last_to_remove = head;
+    do {
+        tag = srwlock_get_tag(old);
+        last = srwlock_get_waiter(old);
+
+        /* Every time cmpxchg failed, new waiter could have been added
+         * to the end of the list. Which means we might have more waiters
+         * to wake, and more head pointers to update. */
+        while (TRUE) {
+            struct srwlock_waiter *tmp =
+                ReadAcquirePointer((void **)&last_to_remove->next);
+            if (tmp != new_head) {
+                new_head = tmp;
+                last_up_to_date = last_to_remove;
+            }
+            if (!new_head)
+                break;
+            if (last_to_remove->state & SRWLOCK_WAITER_STATE_IS_EXCLUSIVE) {
+                new_head->num_owners = SRWLOCK_WAITER_EXCLUSIVELY_LOCKED;
+                break;
+            }
+            new_head->num_owners = last_to_remove->num_owners + 1;
+            if (new_head->num_owners > 1)
+                tag |= SRWLOCK_TAG_HAS_MULTIPLE_OWNERS;
+            if (new_head->state & SRWLOCK_WAITER_STATE_IS_EXCLUSIVE)
+                break;
+            last_to_remove = new_head;
+        }
 
-        do
-        {
-            old.s = *u.s;
-            new.s = old.s;
+        while (last_up_to_date != last) {
+            last_up_to_date = ReadAcquirePointer((void **)&last_up_to_date->next);
+            last_up_to_date->head = new_head;
+        }
 
-            if (!old.s.owners)
-            {
-                /* Not locked exclusive or shared. We can try to grab it. */
-                new.s.owners = -1;
-                --new.s.exclusive_waiters;
-                wait = FALSE;
-            }
+        tag = SRWLOCK_TAG_LOCKED;
+        if (new_head) {
+            tag |= SRWLOCK_TAG_HAS_WAITERS;
+            if (new_head->num_owners > 1 &&
+                new_head->num_owners != SRWLOCK_WAITER_EXCLUSIVELY_LOCKED)
+                tag |= SRWLOCK_TAG_HAS_MULTIPLE_OWNERS;
+            new = srwlock_from_waiter(last, tag);
+        } else {
+            if (last_to_remove->state & SRWLOCK_WAITER_STATE_IS_EXCLUSIVE)
+                new = srwlock_from_shared_owner_count(0, tag);
             else
-            {
-                wait = TRUE;
-            }
-        } while (InterlockedCompareExchange( u.l, new.l, old.l ) != old.l);
+                new = srwlock_from_shared_owner_count(last_to_remove->num_owners + 1, tag);
+        }
 
-        if (!wait) return;
-        RtlWaitOnAddress( &u.s->owners, &new.s.owners, sizeof(short), NULL );
+        read.Ptr = InterlockedCompareExchangePointer(&lock->Ptr, new.Ptr, old.Ptr);
+        if (read.Ptr == old.Ptr)
+            break;
+        old = read;
+    } while (TRUE);
+
+    while (TRUE) {
+        /* waking up the waiter will invalidate the waiter's entry in
+         * the list. */
+        struct srwlock_waiter *next = head->next;
+        InterlockedOr((LONG *)&head->state, SRWLOCK_WAITER_STATE_NOTIFIED);
+        RtlWakeAddressSingle(&head->state);
+        if (head == last_to_remove)
+            break;
+        head = next;
     }
 }
 
-/***********************************************************************
- *              RtlAcquireSRWLockShared (NTDLL.@)
+/**
+ * Add a new waiter to the SRWLOCK, and wait on it. The wait will be interrupted
+ * if the value stored in `lock` changes to something other than `expected`.
  *
- * NOTES
- *   Do not call this function recursively - it will only succeed when
- *   there are no threads waiting for an exclusive lock!
+ * Returns TRUE if the lock is acquired, FALSE otherwise.
  */
-void WINAPI RtlAcquireSRWLockShared( RTL_SRWLOCK *lock )
+static BOOLEAN srwlock_wait(RTL_SRWLOCK *lock, DWORD waiter_state,
+                            RTL_SRWLOCK expected)
 {
-    union { RTL_SRWLOCK *rtl; struct srw_lock *s; LONG *l; } u = { lock };
-
-    for (;;)
-    {
-        union { struct srw_lock s; LONG l; } old, new;
-        BOOL wait;
+    RTL_SRWLOCK new;
+    USHORT tag;
+    struct srwlock_waiter *prev = NULL, waiter = {0};
+
+    tag  = srwlock_get_tag(expected);
+    TRACE("%p %p %p\n", lock, &waiter, expected.Ptr);
+
+    waiter.state = waiter_state;
+    if (tag & SRWLOCK_TAG_HAS_WAITERS) {
+        prev = srwlock_get_waiter(expected);
+        if (!(tag & SRWLOCK_TAG_LIST_LOCKED))
+            waiter.head = prev->head;
+        else
+            /* some other thread is modifying the list, they
+             * are responsible for setting our head pointer. */
+            waiter.head = NULL;
+        waiter.prev = prev;
+        waiter.num_owners = SRWLOCK_WAITER_IS_NOT_HEAD;
+    } else {
+        waiter.head = &waiter;
+        waiter.num_owners = srwlock_get_shared_count(expected);
+        if (waiter.num_owners > 1)
+            tag |= SRWLOCK_TAG_HAS_MULTIPLE_OWNERS;
+        if (waiter.num_owners == 0)
+            waiter.num_owners = SRWLOCK_WAITER_EXCLUSIVELY_LOCKED;
+        tag |= SRWLOCK_TAG_HAS_WAITERS;
+    }
+    waiter.next = NULL;
 
-        do
+    if (prev) {
+        if (InterlockedCompareExchangePointer((void **)&prev->next,
+                                              (void *)&waiter, NULL) != NULL)
         {
-            old.s = *u.s;
-            new = old;
-
-            if (old.s.owners != -1 && !old.s.exclusive_waiters)
-            {
-                /* Not locked exclusive, and no exclusive waiters.
-                 * We can try to grab it. */
-                ++new.s.owners;
-                wait = FALSE;
-            }
-            else
-            {
-                wait = TRUE;
-            }
-        } while (InterlockedCompareExchange( u.l, new.l, old.l ) != old.l);
-
-        if (!wait) return;
-        RtlWaitOnAddress( u.s, &new.s, sizeof(struct srw_lock), NULL );
+            /* Someone else is also appending to the list, they have linked their
+             * node to `prev`, but hasn't changed lock->Ptr yet. So we wait for
+             * the lock to change and retry. */
+            RtlWaitOnAddress(&lock->Ptr, &expected.Ptr, sizeof(expected.Ptr),
+                             NULL);
+            return FALSE;
+        }
     }
-}
-
-/***********************************************************************
- *              RtlReleaseSRWLockExclusive (NTDLL.@)
- */
-void WINAPI RtlReleaseSRWLockExclusive( RTL_SRWLOCK *lock )
-{
-    union { RTL_SRWLOCK *rtl; struct srw_lock *s; LONG *l; } u = { lock };
-    union { struct srw_lock s; LONG l; } old, new;
 
-    do
+    /* Once we reach here, `prev->next` can't be changed by anyone but us. So
+     * if we fail to change lock->Ptr, we have to reset it so others can append
+     * to the list. */
+    new = srwlock_from_waiter(&waiter, tag);
+    if (InterlockedCompareExchangePointer(&lock->Ptr, new.Ptr, expected.Ptr) !=
+        expected.Ptr)
     {
-        old.s = *u.s;
-        new = old;
-
-        if (old.s.owners != -1) ERR("Lock %p is not owned exclusive!\n", lock);
+        if (prev)
+            WriteReleasePointer((void **)&prev->next, NULL);
+        return FALSE;
+    }
 
-        new.s.owners = 0;
-    } while (InterlockedCompareExchange( u.l, new.l, old.l ) != old.l);
+    RtlWakeAddressSingle(&lock->Ptr);
 
-    if (new.s.exclusive_waiters)
-        RtlWakeAddressSingle( &u.s->owners );
-    else
-        RtlWakeAddressAll( u.s );
+    do {
+        waiter_state = ReadAcquire((LONG *)&waiter.state);
+        if (waiter_state & SRWLOCK_WAITER_STATE_NOTIFIED)
+            break;
+        RtlWaitOnAddress(&waiter.state, &waiter_state, sizeof(DWORD), NULL);
+    } while (TRUE);
+    return TRUE;
 }
 
-/***********************************************************************
- *              RtlReleaseSRWLockShared (NTDLL.@)
- */
-void WINAPI RtlReleaseSRWLockShared( RTL_SRWLOCK *lock )
+static BOOLEAN srwlock_try_acquire_exclusive(RTL_SRWLOCK *lock, RTL_SRWLOCK *old)
 {
-    union { RTL_SRWLOCK *rtl; struct srw_lock *s; LONG *l; } u = { lock };
-    union { struct srw_lock s; LONG l; } old, new;
+    RTL_SRWLOCK new;
+    USHORT tag;
 
-    do
-    {
-        old.s = *u.s;
-        new = old;
+    do {
+        old->Ptr = ReadAcquirePointer(&lock->Ptr);
+        tag = srwlock_get_tag(*old);
 
-        if (old.s.owners == -1) ERR("Lock %p is owned exclusive!\n", lock);
-        else if (!old.s.owners) ERR("Lock %p is not owned shared!\n", lock);
+        if (tag & SRWLOCK_TAG_LOCKED)
+            return FALSE;
 
-        --new.s.owners;
-    } while (InterlockedCompareExchange( u.l, new.l, old.l ) != old.l);
+        new = srwlock_from_shared_owner_count(0, SRWLOCK_TAG_LOCKED);
+    } while (InterlockedCompareExchangePointer(&lock->Ptr, new.Ptr, old->Ptr) !=
+             old->Ptr);
 
-    if (!new.s.owners)
-        RtlWakeAddressSingle( &u.s->owners );
+    RtlWakeAddressSingle(&lock->Ptr);
+    return TRUE;
 }
 
 /***********************************************************************
@@ -642,28 +767,53 @@ void WINAPI RtlReleaseSRWLockShared( RTL_SRWLOCK *lock )
  */
 BOOLEAN WINAPI RtlTryAcquireSRWLockExclusive( RTL_SRWLOCK *lock )
 {
-    union { RTL_SRWLOCK *rtl; struct srw_lock *s; LONG *l; } u = { lock };
-    union { struct srw_lock s; LONG l; } old, new;
-    BOOLEAN ret;
+    RTL_SRWLOCK old;
+    return srwlock_try_acquire_exclusive(lock, &old);
+}
 
-    do
-    {
-        old.s = *u.s;
-        new.s = old.s;
+/***********************************************************************
+ *              RtlAcquireSRWLockExclusive (NTDLL.@)
+ *
+ * NOTES
+ *  Unlike RtlAcquireResourceExclusive this function doesn't allow
+ *  nested calls from the same thread. "Upgrading" a shared access lock
+ *  to an exclusive access lock also doesn't seem to be supported.
+ */
+void WINAPI RtlAcquireSRWLockExclusive( RTL_SRWLOCK *lock )
+{
+    RTL_SRWLOCK old;
 
-        if (!old.s.owners)
-        {
-            /* Not locked exclusive or shared. We can try to grab it. */
-            new.s.owners = -1;
-            ret = TRUE;
-        }
-        else
-        {
-            ret = FALSE;
-        }
-    } while (InterlockedCompareExchange( u.l, new.l, old.l ) != old.l);
+    while (!srwlock_try_acquire_exclusive(lock, &old))
+        if (srwlock_wait(lock, SRWLOCK_WAITER_STATE_IS_EXCLUSIVE, old))
+            break;
+}
 
-    return ret;
+static BOOLEAN srwlock_try_acquire_shared(RTL_SRWLOCK *lock, RTL_SRWLOCK *old)
+{
+    RTL_SRWLOCK new;
+    USHORT tag;
+    size_t shared_count;
+
+    do {
+        old->Ptr = ReadAcquirePointer(&lock->Ptr);
+        tag = srwlock_get_tag(*old);
+        shared_count = srwlock_get_shared_count(*old);
+
+        if (tag & SRWLOCK_TAG_HAS_WAITERS)
+            return FALSE;
+
+        if ((tag & SRWLOCK_TAG_LOCKED) && shared_count == 0)
+            return FALSE;
+
+        /* No waiters to check, and the lock is either locked shared, or not
+         * locked at all. We can try to grab it. */
+        new = srwlock_from_shared_owner_count(shared_count + 1,
+                                              tag | SRWLOCK_TAG_LOCKED);
+    } while (InterlockedCompareExchangePointer(&lock->Ptr, new.Ptr,
+                                               old->Ptr ) != old->Ptr);
+
+    RtlWakeAddressSingle(&lock->Ptr);
+    return TRUE;
 }
 
 /***********************************************************************
@@ -671,29 +821,100 @@ BOOLEAN WINAPI RtlTryAcquireSRWLockExclusive( RTL_SRWLOCK *lock )
  */
 BOOLEAN WINAPI RtlTryAcquireSRWLockShared( RTL_SRWLOCK *lock )
 {
-    union { RTL_SRWLOCK *rtl; struct srw_lock *s; LONG *l; } u = { lock };
-    union { struct srw_lock s; LONG l; } old, new;
-    BOOLEAN ret;
+    RTL_SRWLOCK old;
+    return srwlock_try_acquire_shared(lock, &old);
+}
 
-    do
-    {
-        old.s = *u.s;
-        new.s = old.s;
+/***********************************************************************
+ *              RtlAcquireSRWLockShared (NTDLL.@)
+ *
+ * NOTES
+ *   Do not call this function recursively - it will only succeed when
+ *   there are no threads waiting for an exclusive lock!
+ */
+void WINAPI RtlAcquireSRWLockShared( RTL_SRWLOCK *lock )
+{
+    RTL_SRWLOCK old;
 
-        if (old.s.owners != -1 && !old.s.exclusive_waiters)
-        {
-            /* Not locked exclusive, and no exclusive waiters.
-             * We can try to grab it. */
-            ++new.s.owners;
-            ret = TRUE;
+    while (!srwlock_try_acquire_shared(lock, &old))
+        if (srwlock_wait(lock, 0, old))
+            break;
+}
+
+/***********************************************************************
+ *              RtlReleaseSRWLockExclusive (NTDLL.@)
+ */
+void WINAPI RtlReleaseSRWLockExclusive( RTL_SRWLOCK *lock )
+{
+    RTL_SRWLOCK old, new;
+    USHORT tag;
+
+    TRACE("%p %p\n", lock, lock->Ptr);
+
+    do {
+        old.Ptr = ReadAcquirePointer(&lock->Ptr);
+        tag = srwlock_get_tag(old);
+
+        /* Looks weird but this is how Windows does it. */
+        new.Ptr = (void *)((uintptr_t)old.Ptr - 1);
+    } while (InterlockedCompareExchangePointer(&lock->Ptr, new.Ptr, old.Ptr) !=
+             old.Ptr);
+
+    RtlWakeAddressSingle(&lock->Ptr);
+
+    if (tag & SRWLOCK_TAG_HAS_WAITERS)
+        srwlock_transfer_ownership(lock);
+}
+
+/***********************************************************************
+ *              RtlReleaseSRWLockShared (NTDLL.@)
+ */
+void WINAPI RtlReleaseSRWLockShared( RTL_SRWLOCK *lock )
+{
+    RTL_SRWLOCK old, new;
+    USHORT tag;
+    size_t shared_count;
+    struct srwlock_waiter *last;
+
+    TRACE("%p %p\n", lock, lock->Ptr);
+
+    do {
+        old.Ptr = ReadAcquirePointer(&lock->Ptr);
+        tag = srwlock_get_tag(old);
+        shared_count = srwlock_get_shared_count(old);
+        last = srwlock_get_waiter(old);
+
+        if (!(tag & SRWLOCK_TAG_LOCKED)) {
+            /* interestingly this check is only done for shared locks. */
+            EXCEPTION_RECORD record;
+            record.ExceptionAddress = RtlReleaseSRWLockShared;
+            record.ExceptionCode = STATUS_RESOURCE_NOT_OWNED;
+            record.ExceptionFlags = 0;
+            record.ExceptionRecord = NULL;
+            record.NumberParameters = 0;
+            return RtlRaiseException(&record);
         }
-        else
-        {
-            ret = FALSE;
+
+        if (tag & SRWLOCK_TAG_HAS_WAITERS) {
+            shared_count = InterlockedDecrement((LONG *)&last->head->num_owners);
+            break;
         }
-    } while (InterlockedCompareExchange( u.l, new.l, old.l ) != old.l);
 
-    return ret;
+        if (shared_count - 1 < 2)
+            tag &= ~SRWLOCK_TAG_HAS_MULTIPLE_OWNERS;
+        if (shared_count - 1 == 0)
+            tag &= ~SRWLOCK_TAG_LOCKED;
+        new = srwlock_from_shared_owner_count(shared_count - 1, tag);
+    } while (InterlockedCompareExchangePointer(&lock->Ptr, new.Ptr, old.Ptr) !=
+             old.Ptr);
+
+    RtlWakeAddressSingle(&lock->Ptr);
+
+    if (!(tag & SRWLOCK_TAG_HAS_WAITERS) || shared_count != 0)
+        return;
+
+    /* Only one thread reaches here. */
+    srwlock_transfer_ownership(lock);
 }
 
 /***********************************************************************
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/3504

[PATCH v5 2/2] ntdll: An implementation of SRWLOCK that closer matches Windows'.

[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=135813

Your paranoid android.


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

Report validation errors:
d3dx10_37:d3dx10 crashed (80000101)
d3dx10_42:d3dx10 crashed (80000101)

[Yuxuan Shui (＠yshui)]

I implemented a version of SRWLOCK that looks a bit more like Windows'.

I tried my hardest to make this correct, but given how convoluted this is I am sure there are still problems left. So please help me scrutinize the code.

Honestly Windows' way of doing SRWLOCK is pretty cursed. They store waiters in a _doubly_ linked list, and each node also stores a pointer to the head of the list. The list has to support appending from one end and removing from the other. All these make implementing it with atomics a nightmare. My brain is properly fried right now.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3504#note_41790

[Yuxuan Shui (＠yshui)]

ping @zfigura

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3504#note_41791

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/ntdll/sync.c:
> -     * the latter waits only on the "owners" member. Note then that "owners"
> -     * must not be the first element in the structure.
> +enum srwlock_waiter_state {
> +    SRWLOCK_WAITER_STATE_IS_EXCLUSIVE = 1,
> +    /* ???? = 2, */
> +    SRWLOCK_WAITER_STATE_NOTIFIED = 4,
> +};
> +DECLSPEC_ALIGN(16) struct srwlock_waiter {
> +    /* Note the prev pointer is uninitialized for the list head. */
> +    struct srwlock_waiter *prev;
> +    struct srwlock_waiter *head;
> +    struct srwlock_waiter *next;
> +    DWORD thread_id;
> +#ifdef _WIN64
> +    DWORD padding;
> +#endif
> This could probably be ULONG_PTR or similarly sized integer.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3504#note_41793