Bug 90303

Summary:	ATOMIC_OP_NEEDS_MEMORY_BARRIER doesn't play well with ThreadSanitizer
Product:	cairo	Reporter:	Nathan Froyd <froydnj>
Component:	general	Assignee:	Chris Wilson <chris>
Status:	RESOLVED FIXED	QA Contact:	cairo-bugs mailing list <cairo-bugs>
Severity:	normal
Priority:	medium	CC:	freedesktop
Version:	unspecified
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	patch

Description Nathan Froyd 2015-05-04 15:38:48 UTC

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.

Comment 1 Chris Wilson 2015-05-04 15:45:06 UTC

If you have a patch to migrate over to better compiler intrinsics, please do share! I presume you have modern gcc/clang in mind?

Comment 2 Nathan Froyd 2015-05-04 15:48:28 UTC

I can certainly put together a patch.  What's the minimum supported gcc version?

Comment 3 Chris Wilson 2015-05-04 16:06:09 UTC

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...

Comment 4 Nathan Froyd 2015-05-04 17:58:05 UTC

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.

Comment 5 Chris Wilson 2015-05-04 18:14:26 UTC

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

Comment 6 Behdad Esfahbod 2015-05-04 18:14:57 UTC

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

Comment 7 Nathan Froyd 2015-05-04 18:19:39 UTC

(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.

Comment 8 Behdad Esfahbod 2015-05-04 18:32:46 UTC

Yes, Firefox uses HarfBuzz with no threading primitives and from a single thread.

Patches are welcome as bugzilla attachments or github pull requests.

Comment 9 Nathan Froyd 2015-05-05 16:20:35 UTC

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.

Comment 10 Nathan Froyd 2015-05-18 15:58:40 UTC

Ping.  Is there anything I need to do here to move this forward?

Comment 11 Bryce Harrington 2015-06-04 20:33:09 UTC

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

Comment 12 Bryce Harrington 2015-06-04 20:34:58 UTC

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.