From 630fa82ed0d8bde0014201df84aeed5fcd489667 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 17 Nov 2016 17:17:30 -0500 Subject: [PATCH] gthread: Rework to avoid holding a mutex half the time This code was a persistent source of `-fsanitize=thread` errors when I was trying to use it on OSTree. The problem is that while I think this code is functionally correct, we hold a mutex during the writes, but not the reads, and TSAN (IMO correctly) flags that. Reading this, I don't see a reason we need a mutex at all. At the cost of some small code duplication between posix/win32, we can just pass the data we need down into each implementation. This ends up being notably cleaner I think than the awkward "lock/unlock to serialize" dance. (Minor review changes made by Philip Withnall .) https://gitlab.gnome.org/GNOME/glib/issues/1224 --- glib/gthread-posix.c | 15 +++++++++++++-- glib/gthread-win32.c | 15 +++++++++++++-- glib/gthread.c | 28 ++-------------------------- glib/gthreadprivate.h | 5 ++++- 4 files changed, 32 insertions(+), 31 deletions(-) diff --git a/glib/gthread-posix.c b/glib/gthread-posix.c index 5fff51477..9c0b032e6 100644 --- a/glib/gthread-posix.c +++ b/glib/gthread-posix.c @@ -1148,15 +1148,26 @@ g_system_thread_free (GRealThread *thread) } GRealThread * -g_system_thread_new (GThreadFunc thread_func, +g_system_thread_new (GThreadFunc proxy, gulong stack_size, + const char *name, + GThreadFunc func, + gpointer data, GError **error) { GThreadPosix *thread; + GRealThread *base_thread; pthread_attr_t attr; gint ret; thread = g_slice_new0 (GThreadPosix); + base_thread = (GRealThread*)thread; + base_thread->ref_count = 2; + base_thread->ours = TRUE; + base_thread->thread.joinable = TRUE; + base_thread->thread.func = func; + base_thread->thread.data = data; + base_thread->name = g_strdup (name); posix_check_cmd (pthread_attr_init (&attr)); @@ -1174,7 +1185,7 @@ g_system_thread_new (GThreadFunc thread_func, } #endif /* HAVE_PTHREAD_ATTR_SETSTACKSIZE */ - ret = pthread_create (&thread->system_thread, &attr, (void* (*)(void*))thread_func, thread); + ret = pthread_create (&thread->system_thread, &attr, (void* (*)(void*))proxy, thread); posix_check_cmd (pthread_attr_destroy (&attr)); diff --git a/glib/gthread-win32.c b/glib/gthread-win32.c index 1ad5ece80..89df0daa0 100644 --- a/glib/gthread-win32.c +++ b/glib/gthread-win32.c @@ -429,15 +429,26 @@ g_thread_win32_proxy (gpointer data) } GRealThread * -g_system_thread_new (GThreadFunc func, +g_system_thread_new (GThreadFunc proxy, gulong stack_size, + const char *name, + GThreadFunc func, + gpointer data, GError **error) { GThreadWin32 *thread; + GRealThread *base_thread; guint ignore; thread = g_slice_new0 (GThreadWin32); - thread->proxy = func; + thread->proxy = proxy; + base_thread = (GRealThread*)thread; + base_thread->ref_count = 2; + base_thread->ours = TRUE; + base_thread->thread.joinable = TRUE; + base_thread->thread.func = func; + base_thread->thread.data = data; + base_thread->name = g_strdup (name); thread->handle = (HANDLE) _beginthreadex (NULL, stack_size, g_thread_win32_proxy, thread, 0, &ignore); diff --git a/glib/gthread.c b/glib/gthread.c index 2b7be91dd..66e4a1f66 100644 --- a/glib/gthread.c +++ b/glib/gthread.c @@ -515,8 +515,6 @@ static GSList *g_once_init_list = NULL; static void g_thread_cleanup (gpointer data); static GPrivate g_thread_specific_private = G_PRIVATE_INIT (g_thread_cleanup); -G_LOCK_DEFINE_STATIC (g_thread_new); - /* * g_private_set_alloc0: * @key: a #GPrivate @@ -792,16 +790,8 @@ g_thread_proxy (gpointer data) GRealThread* thread = data; g_assert (data); - - /* This has to happen before G_LOCK, as that might call g_thread_self */ g_private_set (&g_thread_specific_private, data); - /* The lock makes sure that g_thread_new_internal() has a chance to - * setup 'func' and 'data' before we make the call. - */ - G_LOCK (g_thread_new); - G_UNLOCK (g_thread_new); - TRACE (GLIB_THREAD_SPAWNED (thread->thread.func, thread->thread.data, thread->name)); @@ -897,24 +887,10 @@ g_thread_new_internal (const gchar *name, gsize stack_size, GError **error) { - GRealThread *thread; - g_return_val_if_fail (func != NULL, NULL); - G_LOCK (g_thread_new); - thread = g_system_thread_new (proxy, stack_size, error); - if (thread) - { - thread->ref_count = 2; - thread->ours = TRUE; - thread->thread.joinable = TRUE; - thread->thread.func = func; - thread->thread.data = data; - thread->name = g_strdup (name); - } - G_UNLOCK (g_thread_new); - - return (GThread*) thread; + return (GThread*) g_system_thread_new (proxy, stack_size, name, + func, data, error); } /** diff --git a/glib/gthreadprivate.h b/glib/gthreadprivate.h index 75bb3cbf2..b0294af4c 100644 --- a/glib/gthreadprivate.h +++ b/glib/gthreadprivate.h @@ -37,8 +37,11 @@ struct _GRealThread /* system thread implementation (gthread-posix.c, gthread-win32.c) */ void g_system_thread_wait (GRealThread *thread); -GRealThread * g_system_thread_new (GThreadFunc func, +GRealThread * g_system_thread_new (GThreadFunc proxy, gulong stack_size, + const char *name, + GThreadFunc func, + gpointer data, GError **error); void g_system_thread_free (GRealThread *thread);