[PATCH] msvcp90: Reimplement condition variables on top of RtlWaitOnAddress().

[Henri Verbeet]

This is largely the same as the condition variable implementation in ntdll.
There are essentially two reasons for doing this:

    - RtlWaitOnAddress() avoids server calls, compared to the existing keyed
      event implementation.
    - The NtReleaseKeyedEvent() call in _Cnd_signal() can hang if a thread was
      killed inside _Cnd_wait(), for example by an application calling exit().
      Piotr Caban reports this scenario is supposed to work with msvcp140,
      although not necessarily with earlier versions of msvcp. (And indeed,
      native msvcp140 is a workaround for the affected applications.)

Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
---
 dlls/msvcp90/misc.c | 44 ++++++++++++++------------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/dlls/msvcp90/misc.c b/dlls/msvcp90/misc.c
index c7a56ad9e97..3ccb628fa24 100644
--- a/dlls/msvcp90/misc.c
+++ b/dlls/msvcp90/misc.c
@@ -853,19 +853,9 @@ typedef _Cnd_t *_Cnd_arg_t;
 #define CND_T_TO_ARG(c)     (&(c))
 #endif
 
-static HANDLE keyed_event;
-
 void __cdecl _Cnd_init_in_situ(_Cnd_t cnd)
 {
     InitializeConditionVariable(&cnd->cv);
-
-    if(!keyed_event) {
-        HANDLE event;
-
-        NtCreateKeyedEvent(&event, GENERIC_READ|GENERIC_WRITE, NULL, 0);
-        if(InterlockedCompareExchangePointer(&keyed_event, event, NULL) != NULL)
-            NtClose(event);
-    }
 }
 
 int __cdecl _Cnd_init(_Cnd_t *cnd)
@@ -878,51 +868,47 @@ int __cdecl _Cnd_init(_Cnd_t *cnd)
 int __cdecl _Cnd_wait(_Cnd_arg_t cnd, _Mtx_arg_t mtx)
 {
     CONDITION_VARIABLE *cv = &CND_T_FROM_ARG(cnd)->cv;
+    int value = *(int *)&cv->Ptr;
 
-    InterlockedExchangeAdd( (LONG *)&cv->Ptr, 1 );
     _Mtx_unlock(mtx);
-
-    NtWaitForKeyedEvent(keyed_event, &cv->Ptr, FALSE, NULL);
-
+    RtlWaitOnAddress(&cv->Ptr, &value, sizeof(value), NULL);
     _Mtx_lock(mtx);
+
     return 0;
 }
 
 int __cdecl _Cnd_timedwait(_Cnd_arg_t cnd, _Mtx_arg_t mtx, const xtime *xt)
 {
     CONDITION_VARIABLE *cv = &CND_T_FROM_ARG(cnd)->cv;
+    int value = *(int *)&cv->Ptr;
     LARGE_INTEGER timeout;
     NTSTATUS status;
 
-    InterlockedExchangeAdd( (LONG *)&cv->Ptr, 1 );
     _Mtx_unlock(mtx);
-
     timeout.QuadPart = (ULONGLONG)(ULONG)_Xtime_diff_to_millis(xt) * -10000;
-    status = NtWaitForKeyedEvent(keyed_event, &cv->Ptr, FALSE, &timeout);
-    if (status)
-    {
-        if (!interlocked_dec_if_nonzero( (LONG *)&cv->Ptr ))
-            status = NtWaitForKeyedEvent( keyed_event, &cv->Ptr, FALSE, NULL );
-    }
-
+    status = RtlWaitOnAddress(&cv->Ptr, &value, sizeof(value), &timeout);
     _Mtx_lock(mtx);
+
     return status ? CND_TIMEDOUT : 0;
 }
 
 int __cdecl _Cnd_broadcast(_Cnd_arg_t cnd)
 {
     CONDITION_VARIABLE *cv = &CND_T_FROM_ARG(cnd)->cv;
-    LONG val = InterlockedExchange( (LONG *)&cv->Ptr, 0 );
-    while (val-- > 0)
-        NtReleaseKeyedEvent( keyed_event, &cv->Ptr, FALSE, NULL );
+
+    InterlockedIncrement((LONG *)&cv->Ptr);
+    RtlWakeAddressAll(cv);
+
     return 0;
 }
 
 int __cdecl _Cnd_signal(_Cnd_arg_t cnd)
 {
     CONDITION_VARIABLE *cv = &CND_T_FROM_ARG(cnd)->cv;
-    if (interlocked_dec_if_nonzero( (LONG *)&cv->Ptr ))
-        NtReleaseKeyedEvent( keyed_event, &cv->Ptr, FALSE, NULL );
+
+    InterlockedIncrement((LONG *)&cv->Ptr);
+    RtlWakeAddressSingle(cv);
+
     return 0;
 }
 
@@ -1781,8 +1767,6 @@ void init_misc(void *base)
 void free_misc(void)
 {
 #if _MSVCP_VER >= 110
-    if(keyed_event)
-        NtClose(keyed_event);
     HeapFree(GetProcessHeap(), 0, broadcast_at_thread_exit.to_broadcast);
 #endif
 }
-- 
2.20.1

[Zebediah Figura]

FWIW, there are kernel32/kernelbase wrappers for these functions that 
could be used here. I don't know if you ended up skipping them to avoid 
a layer of overhead, though.

[Henri Verbeet]

On Fri, 11 Mar 2022 at 18:06, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> FWIW, there are kernel32/kernelbase wrappers for these functions that
> could be used here. I don't know if you ended up skipping them to avoid
> a layer of overhead, though.

Yes, I'm aware of those. I mostly chose the ntdll variants because I
didn't see much of a point in going through the wrappers, but I could
certainly use the kernel32/kernelbase variants instead if those are
preferred for one reason or another.

[Piotr Caban]

On 3/11/22 17:04, Henri Verbeet wrote:
> This is largely the same as the condition variable implementation in ntdll.
> There are essentially two reasons for doing this:
>
> - RtlWaitOnAddress() avoids server calls, compared to the existing keyed
>        event implementation.
>      - The NtReleaseKeyedEvent() call in _Cnd_signal() can hang if a thread was
>        killed inside _Cnd_wait(), for example by an application calling exit().
>        Piotr Caban reports this scenario is supposed to work with msvcp140,
>        although not necessarily with earlier versions of msvcp. (And indeed,
>        native msvcp140 is a workaround for the affected applications.)
> I've sent a set of patches that tries to fix it differently. It's using 
> _Condition_variable to implement _Cnd_* functions in msvcp110/msvcp120. 
> In msvcp140 CONDITION_VARIABLE and SRWLock is used instead.

It should fix the problem you're describing in msvcp140. 
_Condition_variable implementation is still to be fixed (currently 
Wine's implementation hangs while native crashes).

Thanks,
Piotr

[Henri Verbeet]

On Mon, 14 Mar 2022 at 19:50, Piotr Caban <piotr.caban(a)gmail.com> wrote:
> I've sent a set of patches that tries to fix it differently. It's using
> _Condition_variable to implement _Cnd_* functions in msvcp110/msvcp120.
> In msvcp140 CONDITION_VARIABLE and SRWLock is used instead.
>
> It should fix the problem you're describing in msvcp140.

And indeed it does. Thanks!