GNOME Bugzilla – Bug 774645
TSAN patches
Last modified: 2018-05-24 19:15:41 UTC
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.
Created attachment 340185 [details] [review] [TSAN] gobject: Change assertions to read values via atomics
Created attachment 340193 [details] [review] [TSAN] 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.
Created attachment 340194 [details] [review] [TSAN] 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.
Note the first patch changes the behavior for "too many unreferences" from "spew to stderr" to "abort". If someone cares we can change it.
Review of attachment 340193 [details] [review]: Looks good to me.
Review of attachment 340193 [details] [review]: ::: gobject/gobject.c @@ +3175,3 @@ /* decrement the last reference */ old_ref = g_atomic_int_add (&object->ref_count, -1); + g_assert (old_ref > 0); Actually, no, let’s keep this as a g_return_if_fail() (in this new location). This code path is unfortunately hit too often for it to be sensible to abort here. Many buggy programs continue to work fine after we bail out of a double-unref call, since it’s typically only ever a double-unref and not a triple-unref, and when we bail out the program has already cleared its final pointer to the double-unreffed object.
Review of attachment 340194 [details] [review]: Looks good to me apart from a few nitpicks. A nice cleanup. ::: glib/gthread-posix.c @@ +1145,2 @@ gulong stack_size, + const char *name, Nitpick: Argument alignment. ::: glib/gthread.c @@ -770,3 @@ - */ - G_LOCK (g_thread_new); - G_UNLOCK (g_thread_new); You can also delete the G_LOCK_DEFINE_STATIC (g_thread_new); now, right? ::: glib/gthreadprivate.h @@ +42,2 @@ gulong stack_size, + const char *name, Nitpick: Argument alignment.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1224.