1158871 - TSan: various data races in cairo (_atomic_fetch, _cairo_surface_allocate_unique_id, etc.)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nathan Froyd [:froydnj]]

TSan reports a number of races all related to cairo's atomics support, specifically, this bit:

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-atomic-private.h#62

While we do define things appropriately to make cairo use atomic operations in some places, cairo has this secondary ATOMIC_OP_NEEDS_MEMORY_BARRIER check for whether loads need memory barriers prior to the actual load being performed.  ATOMIC_OP_NEEDS_MEMORY_BARRIER is set by cairo's configure, specifically:

http://cgit.freedesktop.org/cairo/tree/build/aclocal.cairo.m4#n203

This hunk seems suspicious to me, as I would have assumed that ARM needs memory barriers since its ordering properties are not as strong as x86.

In any event, even *with* ATOMIC_OP_NEEDS_MEMORY_BARRIER defined, TSan will still be unhappy with the code as-is, because TSan believes the load in:

     __sync_synchronize ();
     return *x;

is still unordered.  TSan really wants to see the load come from a compiler intrinsic, something like:

     return __atomic_load_n(x, __ATOMIC_SEQ_CST);

for recent GCC/Clang.  There is, unfortunately, no equivalently efficient construct with the __sync* family of builtins.  One would have to do something like:

     return __sync_fetch_and_add(x, 0);

I don't know what our policy on patching our local version of cairo is, how willing upstream would be to making ATOMIC_OP_NEEDS_MEMORY_BARRIER go away or fixing the loads under ATOMIC_OP_NEEDS_MEMORY_BARRIER to be observable to TSan.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

It would be great if you could file an upstream bug about this issue.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> It would be great if you could file an upstream bug about this issue.

Is that comment directed at me or at Lee?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> > It would be great if you could file an upstream bug about this issue.
> 
> Is that comment directed at me or at Lee?

It was directed at you.

Comment 4

[Nathan Froyd [:froydnj]]

Looking a little closer, the _freed_pool_get thing is actually its own bug; I'm going to file a new one for that.

Comment 5

[Nathan Froyd [:froydnj]]

I wrote a patch for upstream, we'll see if it gets committed, and then I guess we can backport?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> I wrote a patch for upstream, we'll see if it gets committed, and then I
> guess we can backport?

Yes.

Comment 7

[Nathan Froyd [:froydnj]]

The upstream patch has been committed...so now we add it to the ever-growing number of patches in gfx/cairo/, document it in the README, apply the patch, and done?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #7)
> The upstream patch has been committed...so now we add it to the ever-growing
> number of patches in gfx/cairo/, document it in the README, apply the patch,
> and done?

Yep.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: use new-style __atomic_* primitives in cairo

Jeff for the cairo part, Ted for the moz.build change.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8616004 [details] [diff] [review]
use new-style __atomic_* primitives in cairo

Review of attachment 8616004 [details] [diff] [review]:
-----------------------------------------------------------------

I think you could have skipped build peer review for this, but no big deal.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73d75d149938

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/73d75d149938