Open Bug 844755 Opened 7 years ago Updated 3 years ago

TSan: Thread data race in js::gc::ChunkBitmap::isMarked(js::gc::Cell const*, unsigned int) vs. js::gc::ChunkBitmap::getMarkWordAndMask

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

People

(Reporter: posidron, Assigned: decoder)

References

(Blocks 2 open bugs)

Details

(Keywords: sec-want, Whiteboard: [tsan][leave open])

Attachments

(2 files)

Attached file trace
During Firefox start-up with ThreadSanitizer (LLVM version), we get a data race reported as described in the attached log. Trace was created on mozilla-central revision.

According to the TSan devs, most of the reported traces should be real data races, even though they can be "benign". We need to determine if the race can/should be fixed, or put on the ignore list. Even for benign races, TSan devs suggest to fix them (second priority), as they can also cause problems [1].

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
Tested with m-c changeset: 122820:c233837cce08
Summary: TSan: Thread data race in js/src/gc/Heap.h:675 js::gc::ChunkBitmap::isMarked(js::gc::Cell const*, unsigned int) → TSan: Thread data race in js::gc::ChunkBitmap::isMarked(js::gc::Cell const*, unsigned int) vs. js::gc::ChunkBitmap::getMarkWordAndMask
Ccing bhackett and billm for some help here :)
Would help if I actually Cced the people I wanted to Cc ^^
UnmarkGray called on main thread while js::gc::ArenaLists::backgroundFinalize is called on GC thread.
Assignee: nobody → general
Component: General → JavaScript Engine
(In reply to Andrew McCreight [:mccr8] from comment #4)

backgroundFinalize checks whether cells are marked, and since both black and gray colors are considered marked, I don't think this is a problem.
Thanks for your analysis Jon. I'll try to come up with a patch that marks these functions as to be ignored by TSan. In order to do that, I need to finish bug 847350 first though, because some GC functions are forced to be inlined. With TSan, we need to mark those as never inline if we want to ignore them.
Depends on: 847350
Assignee: general → choller
This patch marks the isMarked function as blacklisted for TSan (which will also disable inlining for that function when building with ThreadSanitizer).
Attachment #818971 - Flags: review?(jcoppeard)
(In reply to Christian Holler (:decoder) from comment #7)

As I understand it, this will disable detection of any races that involve checking whether something is marked on one thread while changing the marking on the other thread.  It would be good if we could still catch other races somehow.

Is it possible to specify that just this specific race (between UnmarkGray and is Marked) is ok?

Since blacklisting is per-function I guess it would help if we had a separate path for checking gray marking too.
(In reply to Jon Coppeard (:jonco) from comment #8)
> (In reply to Christian Holler (:decoder) from comment #7)
> 
> As I understand it, this will disable detection of any races that involve
> checking whether something is marked on one thread while changing the
> marking on the other thread.  It would be good if we could still catch other
> races somehow.
> 
> Is it possible to specify that just this specific race (between UnmarkGray
> and is Marked) is ok?

That would only be possible with the runtime suppression, at the cost of being very expensive. I would rather not want to do that.
(In reply to Christian Holler (:decoder) from comment #9)

As discussed on IRC, let's blacklist this for now and investigate further in this bug.
Attachment #818971 - Flags: review?(jcoppeard) → review+
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a9763f53d9e1

Leaving open per comment 10.
Whiteboard: [tsan][tsan-test-blocker] → [tsan][leave open]
Blocks: 1367103
You need to log in before you can comment on or make changes to this bug.