[PATCH v10 0/1] MR4372: kernel32: impl sync barrier

[quininer (＠quininer)]

This is my first time contributing wine and there are a lot of things I'm not sure about, so any comments are welcome. :)

--
  v10: kernel32: impl sync barrier

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

[PATCH v10 1/1] kernel32: impl sync barrier

[quininer]

From: quininer

---
 dlls/kernel32/kernel32.spec     |  3 ++
 dlls/kernel32/tests/sync.c      | 24 ++++++++++++
 dlls/kernelbase/kernelbase.spec |  6 +--
 dlls/kernelbase/sync.c          | 25 +++++++++++++
 dlls/ntdll/ntdll.spec           |  3 ++
 dlls/ntdll/sync.c               | 66 +++++++++++++++++++++++++++++++++
 include/synchapi.h              | 14 +++++++
 include/winnt.h                 |  8 ++++
 8 files changed, 146 insertions(+), 3 deletions(-)

diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
index 1deeb2a2ca5..081147d827d 100644
--- a/dlls/kernel32/kernel32.spec
+++ b/dlls/kernel32/kernel32.spec
@@ -375,6 +375,7 @@
 @ stdcall DisassociateCurrentThreadFromCallback(ptr) NTDLL.TpDisassociateCallback
 @ stdcall DiscardVirtualMemory(ptr long) kernelbase.DiscardVirtualMemory
 @ stdcall DeleteTimerQueue(long)
+@ stdcall -import DeleteSynchronizationBarrier(ptr)
 @ stdcall -import DeleteTimerQueueEx(long long)
 @ stdcall -import DeleteTimerQueueTimer(long long long)
 @ stdcall -arch=win64 DeleteUmsCompletionList(ptr)
@@ -398,6 +399,7 @@
 @ stdcall EndUpdateResourceA(long long)
 @ stdcall EndUpdateResourceW(long long)
 @ stdcall EnterCriticalSection(ptr) NTDLL.RtlEnterCriticalSection
+@ stdcall -import EnterSynchronizationBarrier(ptr long)
 @ stdcall EnumCalendarInfoA(ptr long long long)
 @ stdcall EnumCalendarInfoExA(ptr long long long)
 @ stdcall -import EnumCalendarInfoExEx(ptr wstr long wstr long long)
@@ -970,6 +972,7 @@
 @ stdcall -import InitializeProcThreadAttributeList(ptr long long ptr)
 @ stdcall InitializeSListHead(ptr) NTDLL.RtlInitializeSListHead
 @ stdcall InitializeSRWLock(ptr) NTDLL.RtlInitializeSRWLock
+@ stdcall -import InitializeSynchronizationBarrier(ptr long long)
 @ stdcall -arch=i386 InterlockedCompareExchange (ptr long long)
 @ stdcall -arch=i386 -ret64 InterlockedCompareExchange64(ptr int64 int64) NTDLL.RtlInterlockedCompareExchange64
 @ stdcall -arch=i386 InterlockedDecrement(ptr)
diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c
index 56a9d6e4859..b45916711ba 100644
--- a/dlls/kernel32/tests/sync.c
+++ b/dlls/kernel32/tests/sync.c
@@ -2841,6 +2841,29 @@ static void test_QueueUserAPC(void)
     ok(apc_count == 1, "APC count %u\n", apc_count);
 }

+static DWORD WINAPI barrier_worker(LPVOID b) {
+    SYNCHRONIZATION_BARRIER *barrier = (SYNCHRONIZATION_BARRIER*)b;
+    EnterSynchronizationBarrier(barrier, 0);
+    return 0;
+}
+
+static void test_barrier(void)
+{
+    SYNCHRONIZATION_BARRIER barrier;
+    DWORD dummy;
+    DWORD r;
+
+    r = InitializeSynchronizationBarrier(&barrier, 3, 0);
+    ok( r == TRUE, "init sync barrier failed\n");
+
+    CreateThread(NULL, 0, barrier_worker, &barrier, 0, &dummy);
+    CreateThread(NULL, 0, barrier_worker, &barrier, 0, &dummy);
+
+    EnterSynchronizationBarrier(&barrier, 0);
+    r = DeleteSynchronizationBarrier(&barrier);
+    ok( r == TRUE, "delete sync barrier failed\n");
+}
+
 START_TEST(sync)
 {
     char **argv;
@@ -2912,4 +2935,5 @@ START_TEST(sync)
     test_alertable_wait();
     test_apc_deadlock();
     test_crit_section();
+    test_barrier();
 }
diff --git a/dlls/kernelbase/kernelbase.spec b/dlls/kernelbase/kernelbase.spec
index e85f2b67d74..c3c6d8ed0dd 100644
--- a/dlls/kernelbase/kernelbase.spec
+++ b/dlls/kernelbase/kernelbase.spec
@@ -260,7 +260,7 @@
 # @ stub DeleteStateAtomValue
 # @ stub DeleteStateContainer
 # @ stub DeleteStateContainerValue
-# @ stub DeleteSynchronizationBarrier
+@ stdcall DeleteSynchronizationBarrier(ptr)
 @ stdcall DeleteTimerQueueEx(long long)
 @ stdcall DeleteTimerQueueTimer(long long long)
 @ stdcall DeleteVolumeMountPointW(wstr)
@@ -293,7 +293,7 @@
 @ stdcall EncodeSystemPointer(ptr) ntdll.RtlEncodeSystemPointer
 # @ stub EnterCriticalPolicySectionInternal
 @ stdcall EnterCriticalSection(ptr) ntdll.RtlEnterCriticalSection
-# @ stub EnterSynchronizationBarrier
+@ stdcall EnterSynchronizationBarrier(ptr long)
 @ stdcall EnumCalendarInfoExEx(ptr wstr long wstr long long)
 @ stdcall EnumCalendarInfoExW(ptr long long long)
 @ stdcall EnumCalendarInfoW(ptr long long long)
@@ -843,7 +843,7 @@
 @ stdcall InitializeSRWLock(ptr) ntdll.RtlInitializeSRWLock
 @ stdcall InitializeSecurityDescriptor(ptr long)
 @ stdcall InitializeSid(ptr ptr long)
-# @ stub InitializeSynchronizationBarrier
+@ stdcall InitializeSynchronizationBarrier(ptr long long)
 # @ stub InstallELAMCertificateInfo
 @ stdcall -arch=i386 InterlockedCompareExchange(ptr long long)
 @ stdcall -arch=i386 -ret64 InterlockedCompareExchange64(ptr int64 int64) ntdll.RtlInterlockedCompareExchange64
diff --git a/dlls/kernelbase/sync.c b/dlls/kernelbase/sync.c
index b2086207caa..a7943778891 100644
--- a/dlls/kernelbase/sync.c
+++ b/dlls/kernelbase/sync.c
@@ -1684,6 +1684,31 @@ BOOL WINAPI DECLSPEC_HOTPATCH InitOnceExecuteOnce( INIT_ONCE *once, PINIT_ONCE_F
     return !RtlRunOnceExecuteOnce( once, (PRTL_RUN_ONCE_INIT_FN)func, param, context );
 }

+/***********************************************************************
+ *           InitializeSynchronizationBarrier    (kernelbase.@)
+ */
+BOOL WINAPI DECLSPEC_HOTPATCH InitializeSynchronizationBarrier( LPSYNCHRONIZATION_BARRIER barrier, LONG total_threads,
+                                                                LONG spin_count )
+{
+    return set_ntstatus( RtlInitBarrier( barrier, total_threads, spin_count ));
+}
+
+/***********************************************************************
+ *           EnterSynchronizationBarrier    (kernelbase.@)
+ */
+BOOL WINAPI DECLSPEC_HOTPATCH EnterSynchronizationBarrier( LPSYNCHRONIZATION_BARRIER barrier, DWORD flags )
+{
+    return RtlBarrier(barrier, flags);
+}
+
+/***********************************************************************
+ *           DeleteSynchronizationBarrier    (kernelbase.@)
+ */
+BOOL WINAPI DECLSPEC_HOTPATCH DeleteSynchronizationBarrier( LPSYNCHRONIZATION_BARRIER barrier )
+{
+    return set_ntstatus( RtlDeleteBarrier( barrier ));
+}
+
 #ifdef __i386__

/***********************************************************************
diff --git a/dlls/ntdll/ntdll.spec b/dlls/ntdll/ntdll.spec
index c09157a369b..f9eac0744ef 100644
--- a/dlls/ntdll/ntdll.spec
+++ b/dlls/ntdll/ntdll.spec
@@ -514,6 +514,7 @@
 @ stdcall RtlAreBitsSet(ptr long long)
 # @ stub RtlAssert2
 @ stdcall RtlAssert(ptr ptr long str)
+@ stdcall RtlBarrier(ptr long)
 # @ stub RtlCancelTimer
 @ stdcall -norelay RtlCaptureContext(ptr)
 @ stdcall RtlCaptureStackBackTrace(long long ptr ptr)
@@ -598,6 +599,7 @@
 @ stub RtlDelete
 @ stdcall RtlDeleteAce(ptr long)
 @ stdcall RtlDeleteAtomFromAtomTable(ptr long)
+@ stdcall RtlDeleteBarrier(ptr)
 @ stdcall RtlDeleteCriticalSection(ptr)
 @ stdcall -arch=!i386 RtlDeleteGrowableFunctionTable(ptr)
 @ stub RtlDeleteElementGenericTable
@@ -777,6 +779,7 @@
 @ stdcall RtlImpersonateSelf(long)
 @ stdcall RtlInitAnsiString(ptr str)
 @ stdcall RtlInitAnsiStringEx(ptr str)
+@ stdcall RtlInitBarrier(ptr long long)
 @ stdcall RtlInitCodePageTable(ptr ptr)
 # @ stub RtlInitMemoryStream
 @ stdcall RtlInitNlsTables(ptr ptr ptr ptr)
diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c
index bb5dcbb66e6..8ef5e1ef56e 100644
--- a/dlls/ntdll/sync.c
+++ b/dlls/ntdll/sync.c
@@ -1000,3 +1000,69 @@ void WINAPI RtlWakeAddressSingle( const void *addr )

if (tid) NtAlertThreadByThreadId( (HANDLE)(DWORD_PTR)tid );
 }
+
+
+typedef struct {
+    RTL_SRWLOCK lock;
+    RTL_CONDITION_VARIABLE cond;
+    ULONG count;
+} barrier_entry;
+C_ASSERT( sizeof(barrier_entry) <= sizeof(RTL_BARRIER) );
+
+/***********************************************************************
+ *           RtlInitBarrier     (NTDLL.@)
+ */
+NTSTATUS WINAPI RtlInitBarrier( PRTL_BARRIER Barrier, ULONG TotalThreads, ULONG SpinCount )
+{
+    if(!Barrier)
+        return STATUS_INVALID_PARAMETER;
+
+    barrier_entry *barrier = (barrier_entry*)Barrier;
+
+    RtlInitializeSRWLock(&barrier->lock);
+    RtlInitializeConditionVariable(&barrier->cond);
+    barrier->count = TotalThreads;
+
+    return STATUS_SUCCESS;
+}
+
+/***********************************************************************
+ *           RtlBarrier         (NTDLL.@)
+ */
+BOOLEAN WINAPI RtlBarrier( PRTL_BARRIER Barrier, ULONG Flags )
+{
+    barrier_entry *barrier = (barrier_entry*)Barrier;
+    BOOL wait = TRUE;
+    BOOL hint = FALSE;
+
+    RtlAcquireSRWLockExclusive(&barrier->lock);
+
+    if (barrier->count > 0) {
+        barrier->count -= 1;
+    }
+
+    if (barrier->count == 0) {
+        RtlWakeAllConditionVariable(&barrier->cond);
+        wait = FALSE;
+        hint = TRUE;
+    }
+
+    while (wait) {
+        RtlSleepConditionVariableSRW(&barrier->cond, &barrier->lock, NULL, 0);
+        wait = barrier->count > 0;
+    }
+
+    RtlReleaseSRWLockExclusive(&barrier->lock);
+
+    return hint;
+}
+
+/***********************************************************************
+ *           RtlDeleteBarrier   (NTDLL.@)
+ */
+NTSTATUS WINAPI RtlDeleteBarrier( PRTL_BARRIER Barrier )
+{
+    if(!Barrier)
+        return STATUS_INVALID_PARAMETER;
+    return STATUS_SUCCESS;
+}
diff --git a/include/synchapi.h b/include/synchapi.h
index 0b3dcb8ff74..2c33907da6b 100644
--- a/include/synchapi.h
+++ b/include/synchapi.h
@@ -19,6 +19,8 @@
 #ifndef _SYNCHAPI_H
 #define _SYNCHAPI_H

+#include <winbase.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -27,6 +29,18 @@ BOOL WINAPI WaitOnAddress(volatile void*, void*, SIZE_T, DWORD);
 void WINAPI WakeByAddressAll(void*);
 void WINAPI WakeByAddressSingle(void*);

+typedef RTL_BARRIER SYNCHRONIZATION_BARRIER;
+typedef PRTL_BARRIER PSYNCHRONIZATION_BARRIER;
+typedef PRTL_BARRIER LPSYNCHRONIZATION_BARRIER;
+
+#define SYNCHRONIZATION_BARRIER_FLAGS_SPIN_ONLY 0x01
+#define SYNCHRONIZATION_BARRIER_FLAGS_BLOCK_ONLY 0x02
+#define SYNCHRONIZATION_BARRIER_FLAGS_NO_DELETE 0x04
+
+BOOL WINAPI InitializeSynchronizationBarrier(LPSYNCHRONIZATION_BARRIER lpBarrier, LONG lTotalThreads, LONG lSpinCount);
+BOOL WINAPI EnterSynchronizationBarrier(LPSYNCHRONIZATION_BARRIER lpBarrier, DWORD dwFlags);
+BOOL WINAPI DeleteSynchronizationBarrier(LPSYNCHRONIZATION_BARRIER lpBarrier);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/include/winnt.h b/include/winnt.h
index d1c82084f9f..7983b1eea4d 100644
--- a/include/winnt.h
+++ b/include/winnt.h
@@ -6097,6 +6097,14 @@ NTSYSAPI DWORD WINAPI RtlRunOnceExecuteOnce(PRTL_RUN_ONCE,PRTL_RUN_ONCE_INIT_FN,
 NTSYSAPI DWORD WINAPI RtlRunOnceBeginInitialize(PRTL_RUN_ONCE, DWORD, PVOID*);
 NTSYSAPI DWORD WINAPI RtlRunOnceComplete(PRTL_RUN_ONCE, DWORD, PVOID);

+typedef struct _RTL_BARRIER {
+    DWORD Reserved1;
+    DWORD Reserved2;
+    ULONG_PTR Reserved3[2];
+    DWORD Reserved4;
+    DWORD Reserved5;
+} RTL_BARRIER, *PRTL_BARRIER;
+
 #include <pshpack8.h>
 typedef struct _IO_COUNTERS {
     ULONGLONG DECLSPEC_ALIGN(8) ReadOperationCount;

[PATCH v10 1/1] kernel32: impl sync barrier

[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 full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=139871

Your paranoid android.

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

kernel32:
sync.c:2864: Test failed: delete sync barrier failed

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

kernel32:
sync.c:2864: Test failed: delete sync barrier failed

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

kernel32:
sync.c:2864: Test failed: delete sync barrier failed

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

kernel32:
sync.c:2864: Test failed: delete sync barrier failed

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

kernel32:
sync.c:2864: Test failed: delete sync barrier failed

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

kernel32:
sync.c:2864: Test failed: delete sync barrier failed

=== debian11 (build log) ===

/home/winetest/tools/testbot/var/wine-win32/../wine/dlls/kernelbase/sync.c:1693: undefined reference to `RtlInitBarrier'
/home/winetest/tools/testbot/var/wine-win32/../wine/dlls/kernelbase/sync.c:1701: undefined reference to `RtlBarrier'
/home/winetest/tools/testbot/var/wine-win32/../wine/dlls/kernelbase/sync.c:1709: undefined reference to `RtlDeleteBarrier'
collect2: error: ld returned 1 exit status
Task: The win32 Wine build failed

=== debian11b (build log) ===

/home/winetest/tools/testbot/var/wine-wow32/../wine/dlls/kernelbase/sync.c:1693: undefined reference to `RtlInitBarrier'
/home/winetest/tools/testbot/var/wine-wow32/../wine/dlls/kernelbase/sync.c:1701: undefined reference to `RtlBarrier'
/home/winetest/tools/testbot/var/wine-wow32/../wine/dlls/kernelbase/sync.c:1709: undefined reference to `RtlDeleteBarrier'
collect2: error: ld returned 1 exit status
Task: The wow32 Wine build failed

[quininer (＠quininer)]

I didn't know how wine's `kernelbase.spec` stdcall would handle functions forwarded from ntdll, so I manually forwarded the code like @etaash.mathamsetty .

I keep internally defined `barrier_entry` because I thought the `RTL_BARRIER` would be best to keep it consistent with original header file.

[Etaash Mathamsetty (＠etaash.mathamsetty)]

Etaash Mathamsetty (@etaash.mathamsetty) commented about dlls/kernel32/tests/sync.c:
> +static DWORD WINAPI barrier_worker(LPVOID b) {
> - SYNCHRONIZATION_BARRIER *barrier = (SYNCHRONIZATION_BARRIER*)b;
> - EnterSynchronizationBarrier(barrier, 0);
> - return 0;
>
> +}
> -
>
> +static void test_barrier(void)
> +{
> - SYNCHRONIZATION_BARRIER barrier;
> - DWORD dummy;
> - DWORD r;
> -
> - r = InitializeSynchronizationBarrier(&barrier, 3, 0);
> - ok( r == TRUE, "init sync barrier failed\n");
> -
> - CreateThread(NULL, 0, barrier_worker, &barrier, 0, &dummy);

Generally, it's better to have tests in a seperate commit. 
It would also be nice to have a test to see what happens when the barrier is reused, that bakin engine you linked seems to be reusing them, so it would be good to test the behavior in that situation.