1141563 - TSan: data race js/src/vm/Shape.h:1424 getter (and line 1430, setter)

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: js-gc-fixup-race.txt

The attached logfile shows a thread/data race detected by TSan (ThreadSanitizer).

* Specific information about this bug

Given that the offending functions for the read side of the race are single lines:

inline GetterOp
Shape::getter() const
{
    return isAccessorShape() ? asAccessorShape().rawGetter : nullptr;
}

inline SetterOp
Shape::setter() const
{
    return isAccessorShape() ? asAccessorShape().rawSetter : nullptr;
}

I *think* the race is on the raw{Getter,Setter} slots.  (The |flags| slot, if I'm doing my math correctly, would reside on an odd address, and the offending address is an even address; the access size is also 8, which rules out |flags|.)  But that doesn't make any sense looking at the offending write:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp#2256

since raw{Getter,Setter} are a) presumably not tracked by the GC; and b) are not JSObjects in any event, so I would think they would not be touched by |MovingTracer::Visit|.  I'm not entirely sure what TSan is complaining about here...Terrence?

* General information about TSan, data races, etc.

Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause unacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1][2].

If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[2] _How to miscompile programs with "benign" data races_: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #0)
> since raw{Getter,Setter} are a) presumably not tracked by the GC; and b) are
> not JSObjects in any event,

rawGetter/rawSetter can be JSObjects just fine. We have two kinds of getter/setter hooks in the engine, the old property ops and JSObjects, and both can be stuffed in those fields...

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #0)
> > since raw{Getter,Setter} are a) presumably not tracked by the GC; and b) are
> > not JSObjects in any event,
> 
> rawGetter/rawSetter can be JSObjects just fine. We have two kinds of
> getter/setter hooks in the engine, the old property ops and JSObjects, and
> both can be stuffed in those fields...

Aha!  My mistake, I took the declarations in StackShape as definitive and didn't look closely enough at AccessorShape.

Given that, it's possible that this is just the version of TSan that I'm using complaining about the transformation:

  x ? *y : nullptr;

to:

  tmp = *y;
  x ? tmp : nullptr; // done branchlessly via bit-twiddling

I'd have to look at the assembly to make sure.  They've fixed llvm to not do this transformation when TSan is in use (!) but I've not been able to make a version with that fix work properly with Firefox.

Comment 3

[Terrence Cole [:terrence]]

Jon, could you take a closer look at this?

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1141563-dont-update-shapes-in-parallel

Oh, this is because Shape::fixupShapeTreeAfterMovingGC() accesses child shape accessors, which may be in the process of being updated on another thread.

It's probably safest to not update shapes in parallel.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8580794 [details] [diff] [review]
bug1141563-dont-update-shapes-in-parallel

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

Nice comment!

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5850aa2851fe

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5850aa2851fe