[PATCH 0/9] MR9023: libs/fluidsynth: Miscellaneous fixes in glib stubs.
Mostly for vendoring of future fluidsynth versions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Fixes: f768d6b31bebc35fbaf751d0cd57c8bd302a8d60 --- libs/fluidsynth/glib.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/libs/fluidsynth/glib.c b/libs/fluidsynth/glib.c index cd938a4fb6b..2450924eb78 100644 --- a/libs/fluidsynth/glib.c +++ b/libs/fluidsynth/glib.c @@ -40,10 +40,14 @@ int g_snprintf( char *buffer, size_t size, const char *format, ... ) double g_get_monotonic_time(void) { - static LARGE_INTEGER frequency = {0}; - LARGE_INTEGER counter; + static volatile LONG64 freq_cache; + LARGE_INTEGER counter, frequency; - if (!frequency.QuadPart) QueryPerformanceFrequency( &frequency ); + if (!(frequency.QuadPart = ReadAcquire64( &freq_cache ))) + { + QueryPerformanceFrequency( &frequency ); + WriteRelease64( &freq_cache, frequency.QuadPart ); + } QueryPerformanceCounter( &counter ); return counter.QuadPart * 1000000.0 / frequency.QuadPart; /* time in micros */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Fixes: f768d6b31bebc35fbaf751d0cd57c8bd302a8d60 --- libs/fluidsynth/glib.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/fluidsynth/glib.h b/libs/fluidsynth/glib.h index a2cfd2c743d..04b9fb82d54 100644 --- a/libs/fluidsynth/glib.h +++ b/libs/fluidsynth/glib.h @@ -81,7 +81,7 @@ extern int g_file_test( const char *path, int test ); static inline void g_free( void *ptr ) { free( ptr ); } typedef SRWLOCK GMutex; -static inline void g_mutex_init( GMutex *mutex ) {} +static inline void g_mutex_init( GMutex *mutex ) { InitializeSRWLock( mutex ); } static inline void g_mutex_clear( GMutex *mutex ) {} static inline void g_mutex_lock( GMutex *mutex ) { AcquireSRWLockExclusive( mutex ); } static inline void g_mutex_unlock( GMutex *mutex ) { ReleaseSRWLockExclusive( mutex ); } @@ -93,7 +93,7 @@ static inline void g_rec_mutex_lock( GRecMutex *mutex ) { EnterCriticalSection( static inline void g_rec_mutex_unlock( GRecMutex *mutex ) { LeaveCriticalSection( mutex ); } typedef CONDITION_VARIABLE GCond; -static inline void g_cond_init( GCond *cond ) {} +static inline void g_cond_init( GCond *cond ) { InitializeConditionVariable( cond ); } static inline void g_cond_clear( GCond *cond ) {} static inline void g_cond_signal( GCond *cond ) { WakeConditionVariable( cond ); } static inline void g_cond_broadcast( GCond *cond ) { WakeAllConditionVariable( cond ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Fixes: f768d6b31bebc35fbaf751d0cd57c8bd302a8d60 --- libs/fluidsynth/glib.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/libs/fluidsynth/glib.c b/libs/fluidsynth/glib.c index 2450924eb78..bdab5505daa 100644 --- a/libs/fluidsynth/glib.c +++ b/libs/fluidsynth/glib.c @@ -62,7 +62,7 @@ static DWORD CALLBACK g_thread_wrapper( void *args ) { GThread *thread = args; gpointer ret = thread->func( thread->data ); - if (!InterlockedDecrement( &thread->ref )) free( thread ); + g_thread_unref( thread ); return (UINT_PTR)ret; } @@ -86,8 +86,11 @@ GThread *g_thread_try_new( const char *name, GThreadFunc func, gpointer data, GE void g_thread_unref( GThread *thread ) { - CloseHandle( thread->handle ); - if (!InterlockedDecrement( &thread->ref )) free( thread ); + if (!InterlockedDecrement( &thread->ref )) + { + CloseHandle( thread->handle ); + free( thread ); + } } void g_thread_join( GThread *thread ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Fixes: f768d6b31bebc35fbaf751d0cd57c8bd302a8d60 --- libs/fluidsynth/glib.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/fluidsynth/glib.h b/libs/fluidsynth/glib.h index 04b9fb82d54..aeaaf097622 100644 --- a/libs/fluidsynth/glib.h +++ b/libs/fluidsynth/glib.h @@ -101,7 +101,7 @@ static inline void g_cond_wait( GCond *cond, GMutex *mutex ) { SleepConditionVar static inline void g_atomic_int_inc( int *ptr ) { InterlockedIncrement( (LONG *)ptr ); } static inline int g_atomic_int_add( int *ptr, int val ) { return InterlockedAdd( (LONG *)ptr, val ) - val; } -static inline int g_atomic_int_get( int *ptr ) { return ReadAcquire( (LONG *)ptr ); } +static inline int g_atomic_int_get( int *ptr ) { int value = ReadNoFence( (LONG *)ptr ); MemoryBarrier(); return value; } static inline void g_atomic_int_set( int *ptr, int val ) { InterlockedExchange( (LONG *)ptr, val ); } static inline int g_atomic_int_dec_and_test( int *ptr, int val ) { return !InterlockedAdd( (LONG *)ptr, -val ); } static inline int g_atomic_int_compare_and_exchange( int *ptr, int cmp, int val ) { return InterlockedCompareExchange( (LONG *)ptr, val, cmp ) == cmp; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Returning early is never correct; returning late is. Fixes: f768d6b31bebc35fbaf751d0cd57c8bd302a8d60 --- libs/fluidsynth/glib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/fluidsynth/glib.c b/libs/fluidsynth/glib.c index bdab5505daa..7d9c820a57c 100644 --- a/libs/fluidsynth/glib.c +++ b/libs/fluidsynth/glib.c @@ -55,7 +55,7 @@ double g_get_monotonic_time(void) void g_usleep( unsigned int micros ) { - Sleep( micros / 1000 ); + Sleep( (micros + 999) / 1000 ); } static DWORD CALLBACK g_thread_wrapper( void *args ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Fixes: f768d6b31bebc35fbaf751d0cd57c8bd302a8d60 --- libs/fluidsynth/glib.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/libs/fluidsynth/glib.c b/libs/fluidsynth/glib.c index 7d9c820a57c..f358a6a592a 100644 --- a/libs/fluidsynth/glib.c +++ b/libs/fluidsynth/glib.c @@ -107,7 +107,10 @@ void g_clear_error( GError **error ) int g_file_test( const char *path, int test ) { DWORD attrs = GetFileAttributesA( path ); - if (test == G_FILE_TEST_EXISTS) return attrs != INVALID_FILE_ATTRIBUTES; - if (test == G_FILE_TEST_IS_REGULAR) return attrs == FILE_ATTRIBUTE_NORMAL; + if (attrs != INVALID_FILE_ATTRIBUTES) + { + if (test & G_FILE_TEST_EXISTS) return 1; + if ((test & G_FILE_TEST_IS_REGULAR) && !(attrs & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_DEVICE))) return 1; + } return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Fixes: f768d6b31bebc35fbaf751d0cd57c8bd302a8d60 --- libs/fluidsynth/glib.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/fluidsynth/glib.h b/libs/fluidsynth/glib.h index aeaaf097622..3ff3a962fab 100644 --- a/libs/fluidsynth/glib.h +++ b/libs/fluidsynth/glib.h @@ -103,5 +103,5 @@ static inline void g_atomic_int_inc( int *ptr ) { InterlockedIncrement( (LONG *) static inline int g_atomic_int_add( int *ptr, int val ) { return InterlockedAdd( (LONG *)ptr, val ) - val; } static inline int g_atomic_int_get( int *ptr ) { int value = ReadNoFence( (LONG *)ptr ); MemoryBarrier(); return value; } static inline void g_atomic_int_set( int *ptr, int val ) { InterlockedExchange( (LONG *)ptr, val ); } -static inline int g_atomic_int_dec_and_test( int *ptr, int val ) { return !InterlockedAdd( (LONG *)ptr, -val ); } +static inline int g_atomic_int_dec_and_test( int *ptr ) { return !InterlockedAdd( (LONG *)ptr, -1 ); } static inline int g_atomic_int_compare_and_exchange( int *ptr, int cmp, int val ) { return InterlockedCompareExchange( (LONG *)ptr, val, cmp ) == cmp; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Fixes: f768d6b31bebc35fbaf751d0cd57c8bd302a8d60 --- libs/fluidsynth/glib.c | 10 +++++++--- libs/fluidsynth/glib.h | 3 ++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/libs/fluidsynth/glib.c b/libs/fluidsynth/glib.c index f358a6a592a..5353fe99c61 100644 --- a/libs/fluidsynth/glib.c +++ b/libs/fluidsynth/glib.c @@ -61,9 +61,9 @@ void g_usleep( unsigned int micros ) static DWORD CALLBACK g_thread_wrapper( void *args ) { GThread *thread = args; - gpointer ret = thread->func( thread->data ); + thread->result = thread->func( thread->data ); g_thread_unref( thread ); - return (UINT_PTR)ret; + return 0; } GThread *g_thread_try_new( const char *name, GThreadFunc func, gpointer data, GError **err ) @@ -93,10 +93,14 @@ void g_thread_unref( GThread *thread ) } } -void g_thread_join( GThread *thread ) +gpointer g_thread_join( GThread *thread ) { + gpointer result; + WaitForSingleObject( thread->handle, INFINITE ); + result = thread->result; g_thread_unref( thread ); + return result; } void g_clear_error( GError **error ) diff --git a/libs/fluidsynth/glib.h b/libs/fluidsynth/glib.h index 3ff3a962fab..950ce684c19 100644 --- a/libs/fluidsynth/glib.h +++ b/libs/fluidsynth/glib.h @@ -59,6 +59,7 @@ typedef struct HANDLE handle; GThreadFunc func; gpointer data; + gpointer result; } GThread; extern int g_vsnprintf( char *buffer, size_t size, const char *format, va_list args ) __WINE_CRT_PRINTF_ATTR(3, 0); @@ -69,7 +70,7 @@ extern void g_usleep( unsigned int micros ); extern GThread *g_thread_try_new( const char *name, GThreadFunc func, gpointer data, GError **err ); extern void g_thread_unref( GThread *thread ); -extern void g_thread_join( GThread *thread ); +extern gpointer g_thread_join( GThread *thread ); extern void g_clear_error( GError **error ); #define G_FILE_TEST_EXISTS 1 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- libs/fluidsynth/glib.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/fluidsynth/glib.h b/libs/fluidsynth/glib.h index 950ce684c19..466a2644d98 100644 --- a/libs/fluidsynth/glib.h +++ b/libs/fluidsynth/glib.h @@ -101,7 +101,7 @@ static inline void g_cond_broadcast( GCond *cond ) { WakeAllConditionVariable( c static inline void g_cond_wait( GCond *cond, GMutex *mutex ) { SleepConditionVariableSRW( cond, mutex, INFINITE, 0 ); } static inline void g_atomic_int_inc( int *ptr ) { InterlockedIncrement( (LONG *)ptr ); } -static inline int g_atomic_int_add( int *ptr, int val ) { return InterlockedAdd( (LONG *)ptr, val ) - val; } +static inline int g_atomic_int_add( int *ptr, int val ) { return InterlockedExchangeAdd( (LONG *)ptr, val ); } static inline int g_atomic_int_get( int *ptr ) { int value = ReadNoFence( (LONG *)ptr ); MemoryBarrier(); return value; } static inline void g_atomic_int_set( int *ptr, int val ) { InterlockedExchange( (LONG *)ptr, val ); } static inline int g_atomic_int_dec_and_test( int *ptr ) { return !InterlockedAdd( (LONG *)ptr, -1 ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
d3d9:device failure seem unrelated. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023#note_116598
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023
``` + if (!(frequency.QuadPart = ReadAcquire64( &freq_cache ))) + { + QueryPerformanceFrequency( &frequency ); + WriteRelease64( &freq_cache, frequency.QuadPart ); + } QueryPerformanceCounter( &counter ); ``` Why do we need acquire/release here? Where is the barrier pair? ``` -static inline int g_atomic_int_get( int *ptr ) { return ReadAcquire( (LONG *)ptr ); } +static inline int g_atomic_int_get( int *ptr ) { int value = ReadNoFence( (LONG *)ptr ); MemoryBarrier(); return value; } ``` Why? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023#note_116670
Why do we need acquire/release here? Where is the barrier pair?
In the same function. To ensure QueryPerformanceFrequency happens before QueryPerformanceCounter in another thread.
Why?
https://docs.gtk.org/glib/func.atomic_int_get.html says: "This call acts as a full compiler and hardware memory barrier (before the get)." -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023#note_116708
To ensure QueryPerformanceFrequency happens before QueryPerformanceCounter in another thread.
Why? To account for a possible failure mode where [Rtl]QueryPerformanceFrequency and [Rtl]QueryPerformanceCounter are both detoured by an app/system DLL which is order sensitive to either call. You could say "no known apps have done that" but I don't really see what's wrong with preemptly defending against possible race situation, especially since (1) they are hard to debug, and (2) fixing it doesn't introduce extra complexity other than a stronger memory order. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023#note_116709
Why? To account for a possible failure mode where \[Rtl\]QueryPerformanceFrequency and \[Rtl\]QueryPerformanceCounter are both detoured by an app/system DLL which is order sensitive to both calls.
You could say "no known apps have done that" but I don't really see what's wrong with preemptly defending against possible race situation, especially since (1) they are hard to debug, and (2) fixing it doesn't introduce extra complexity other than a stronger memory order.
That seems way too far-fetched. Neither of those functions has side effects, and they're called in either order all the time. Any complexity like this is inherently a problem, because it causes a reader to wonder why it's there. It's not declarative. It's also not a pattern that exists anywhere else in Wine. You could use an argument like this to justify putting MemoryBarrier() literally anywhere. As far as I can see, the only thing we need to prevent is shearing. We don't need barriers at all. Am I missing something?
Why?
https://docs.gtk.org/glib/func.atomic_int_get.html says: "This call acts as a full compiler and hardware memory barrier (before the get)." But the actual code uses \__ATOMIC_SEQ_CST, which is a barrier _after_ the load (as evident in ARM assembly), so I assumed the docs had a mistake.
Thanks, that makes sense. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023#note_116710
On Mon Sep 22 23:48:25 2025 +0000, Elizabeth Figura wrote:
Why? To account for a possible failure mode where \[Rtl\]QueryPerformanceFrequency and \[Rtl\]QueryPerformanceCounter are both detoured by an app/system DLL which is order sensitive to both calls.
You could say "no known apps have done that" but I don't really see what's wrong with preemptly defending against possible race situation, especially since (1) they are hard to debug, and (2) fixing it doesn't introduce extra complexity other than a stronger memory order. That seems way too far-fetched. Neither of those functions has side effects, and they're called in either order all the time. Any complexity like this is inherently a problem, because it causes a reader to wonder why it's there. It's not declarative. It's also not a pattern that exists anywhere else in Wine. You could use an argument like this to justify putting MemoryBarrier() literally anywhere. As far as I can see, the only thing we need to prevent is shearing. We don't need barriers at all. Am I missing something?
Why?
https://docs.gtk.org/glib/func.atomic_int_get.html says: "This call acts as a full compiler and hardware memory barrier (before the get)." But the actual code uses \__ATOMIC_SEQ_CST, which is a barrier _after_ the load (as evident in ARM assembly), so I assumed the docs had a mistake. Thanks, that makes sense. I see, thanks for your input.
FWIW there is no WriteNoFence64, so I'll see what I can do (maybe I could just drop that commit). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023#note_116711
On Mon Sep 22 23:56:41 2025 +0000, Jinoh Kang wrote:
I see, thanks for your input. FWIW there is no WriteNoFence64, so I'll see what I can do (maybe I could just drop that commit). Looks like it exists in native winnt.h, or am I missing something? It shouldn't be difficult to add to Wine I imagine.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023#note_116712
On Tue Sep 23 15:52:00 2025 +0000, Elizabeth Figura wrote:
Looks like it exists in native winnt.h, or am I missing something? It shouldn't be difficult to add to Wine I imagine. I splitted out the first commit as !9048.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9023#note_116784
participants (5)
-
Elizabeth Figura (＠zfigura) -
Huw Davies (＠huw)
-
Jinoh Kang
-
Jinoh Kang (＠iamahuman)
-
Rémi Bernon