ThreadSanitizer (TSan, https://code.google.com/p/thread-sanitizer/) is a tool for detecting data races. Unlike Helgrind (http://www.valgrind.org/info/tools.html), TSan works by having the compiler instrument the code as it's being compiled, which means that it natively understand things like atomic instructions, and requires many less manual annotations than Helgrind. TSan's instrumentation-based approach doesn't work very well with Cairo's atomic ops implementations, particularly _cairo_atomic_{int,ptr}_get: http://cgit.freedesktop.org/cairo/tree/src/cairo-atomic-private.h#n62 For TSan's race detection to understand atomic loads and stores, it really wants to see compiler intrinsics used to handle loads and stores to memory locations. The above implementations of _cairo_atomic_{int,ptr}_get fail in two ways: - For ATOMIC_OP_NEEDS_MEMORY_BARRIER implementations, TSan doesn't see that there's a memory barrier (__sync_synchronize) prior to the load, so it considers the loads after the memory barrier to be synchronized, causing (possibly spurious) data races to be reported. (It's not immediately obvious to me why the barrier is prior to the load; most (all?) non-x86 architectures put a memory barrier *after* the load for a load acquire, and memory barriers before and after the load for sequentially consistent loads.) - For architectures where ATOMIC_OP_NEEDS_MEMORY_BARRIER is false, the loads are completely unsynchronized, again causing spurious data races. TSan would be happy here if the load went through something like __atomic_read_n(x, __ATOMIC_RELAXED) or similar on newer GCC/Clang. Unfortunately, the __sync* family of intrinsics doesn't provide enough control to solve the first issue without some small speed penalty: there are no direct load instructions in the __sync* builtins, so you'd have to do something like __sync_fetch_and_add(x, 0). If the compiler is new enough, it would be great if cairo-atomic-private.h would use the __atomic* family of intrinsics, specifying the intended memory model for these get operations, and then let the compiler take care of generating efficient code for all architectures, rather than the fragile ATOMIC_OP_NEEDS_MEMORY_BARRIER.
If you have a patch to migrate over to better compiler intrinsics, please do share! I presume you have modern gcc/clang in mind?
I can certainly put together a patch. What's the minimum supported gcc version?
You have to assume someone will try compiling on Solaris and complain... Basically do: #if HAS_GCC(5,0) #define COMPILER_HAS_ATOMIC #endif #if COMPILER_HAS_ATOMICS /* Use modern version: even include a lesson on ThreadSanitizer */ #else /* The current file */ #endif Change as required. If the current set of operations are not sufficient, or if there are places in the code not adhering to even the current rules, please do those as follow up patches. They would be good examples of ThreadSantizer at work...
Created attachment 115535 [details] [review] patch Here's a first cut. I haven't run this through TSan yet. Any feedback on coding style, naming, etc. would be appreciated.
Looks good to me. Something that is missing in that file is that we really should do #if HAVE_CXX11_ATOMIC_PRIMITIVES #elif HAVE_INTEL_ATOMIC_PRIMITIVES #elif /* blah */ #endif
Would you also be able to take a look at improving HarfBuzz (and fontconfig, which copied the atomic ops from HarfBuzz; but I can do that part myself): https://bugs.freedesktop.org/show_bug.cgi?id=90305
(In reply to Chris Wilson from comment #5) > Looks good to me. Something that is missing in that file is that we really > should do > > #if HAVE_CXX11_ATOMIC_PRIMITIVES > #elif HAVE_INTEL_ATOMIC_PRIMITIVES > #elif /* blah */ > #endif I could do that as a followup patch. Where do standalone patches like that get sent? A bug in bugzilla, or straight to the mailing list? (In reply to Behdad Esfahbod from comment #6) > Would you also be able to take a look at improving HarfBuzz (and fontconfig, > which copied the atomic ops from HarfBuzz; but I can do that part myself): > > https://bugs.freedesktop.org/show_bug.cgi?id=90305 Sure, I could look at that. I haven't seen any HarfBuzz bits come up when running Firefox through TSan, but it's entirely possible that we only use HarfBuzz on a single thread.
Yes, Firefox uses HarfBuzz with no threading primitives and from a single thread. Patches are welcome as bugzilla attachments or github pull requests.
FWIW, this patch does fix the races seen with _cario_atomic_{int,ptr}_get. So if y'all are happy with it, I think it's ready to go.
Ping. Is there anything I need to do here to move this forward?
Looks good to me, and with Chris' ack in #5 I've gone ahead and pushed to trunk: To ssh://git.cairographics.org/git/cairo d1dda5e..5d150ee master -> master
Thanks for this contribution Nathan. You're welcomed to post follow up patches as new bugzilla tickets, but I tend to keep a closer eye on the mailing list so if you need speedier action on a ticket you can generally catch my attention there.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.