Closed
Bug 1141563
Opened 8 years ago
Closed 8 years ago
TSan: data race js/src/vm/Shape.h:1424 getter (and line 1430, setter)
Categories
(Core :: JavaScript: GC, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: froydnj, Assigned: jonco)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tsan])
Attachments
(2 files)
13.81 KB,
text/plain
|
Details | |
1.60 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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
Flags: needinfo?(terrence)
Comment 1•8 years ago
|
||
(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...
![]() |
Reporter | |
Comment 2•8 years ago
|
||
(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•8 years ago
|
||
Jon, could you take a closer look at this?
Flags: needinfo?(terrence) → needinfo?(jcoppeard)
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8580794 -
Flags: review?(terrence)
Comment 5•8 years ago
|
||
Comment on attachment 8580794 [details] [diff] [review] bug1141563-dont-update-shapes-in-parallel Review of attachment 8580794 [details] [diff] [review]: ----------------------------------------------------------------- Nice comment!
Attachment #8580794 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5850aa2851fe
Comment 7•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5850aa2851fe
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•