From 4631cd892da2ad7ea54912b7f20af33ee2c72744 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 17 Nov 2016 15:43:26 -0500 Subject: [PATCH 1/2] gobject: Change assertions to read values via atomics I'm trying to use `-fsanitize=thread` for OSTree, and some of these issues seem to go into GLib. Also, the sanitizers work better if the userspace libraries are built with them too. This fix is similar to https://github.com/ostreedev/ostree/pull/582/commits/b6814bb37cacd7a36715cf91766eb760b1b33c66 Mixing atomic and non-atomic reads trips TSAN, so let's change the assertions to operate on the local values returned from atomic read/writes. Without this change I couldn't even *build* GLib with TSAN, since we use gresources during compilation, which uses GSubprocess, which hits this code. (Minor review fixes made by Philip Withnall .) https://gitlab.gnome.org/GNOME/glib/issues/1224 --- gobject/gobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index 3f8ad9273..de61a0481 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -3210,9 +3210,9 @@ gpointer gint old_val; g_return_val_if_fail (G_IS_OBJECT (object), NULL); - g_return_val_if_fail (object->ref_count > 0, NULL); old_val = g_atomic_int_add (&object->ref_count, 1); + g_return_val_if_fail (old_val > 0, NULL); if (old_val == 1 && OBJECT_HAS_TOGGLE_REF (object)) toggle_refs_notify (object, FALSE); @@ -3241,7 +3241,6 @@ g_object_unref (gpointer _object) gint old_ref; g_return_if_fail (G_IS_OBJECT (object)); - g_return_if_fail (object->ref_count > 0); /* here we want to atomically do: if (ref_count>1) { ref_count--; return; } */ retry_atomic_decrement1: @@ -3336,6 +3335,7 @@ g_object_unref (gpointer _object) /* decrement the last reference */ old_ref = g_atomic_int_add (&object->ref_count, -1); + g_return_if_fail (old_ref > 0); TRACE (GOBJECT_OBJECT_UNREF(object,G_TYPE_FROM_INSTANCE(object),old_ref)); From 630fa82ed0d8bde0014201df84aeed5fcd489667 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 17 Nov 2016 17:17:30 -0500 Subject: [PATCH 2/2] 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);