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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Christoph Diehl [:posidron]]

Attachment: 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

Comment 1

[Christoph Diehl [:posidron]]

Tested with m-c changeset: 122820:c233837cce08

Comment 2

[Christian Holler (:decoder)]

Ccing bhackett and billm for some help here :)

Comment 3

[Christian Holler (:decoder)]

Would help if I actually Cced the people I wanted to Cc ^^

Comment 4

[Andrew McCreight [:mccr8]]

UnmarkGray called on main thread while js::gc::ArenaLists::backgroundFinalize is called on GC thread.

Comment 5

[Jon Coppeard (:jonco)]

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

Comment 6

[Christian Holler (:decoder)]

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.

Comment 7

[Christian Holler (:decoder)]

Attachment: tsan-gc-ismarked.patch

This patch marks the isMarked function as blacklisted for TSan (which will also disable inlining for that function when building with ThreadSanitizer).

Comment 8

[Jon Coppeard (:jonco)]

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

Comment 9

[Christian Holler (:decoder)]

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

Comment 10

[Jon Coppeard (:jonco)]

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

Comment 11

[Christian Holler (:decoder)]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a9763f53d9e1

Leaving open per comment 10.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a9763f53d9e1

Comment 13

[Jon Coppeard (:jonco)]

Attachment: Bug 844755 - Make mark bitmaps atomic r?sfink

Currently there is a "benign" race between background sweeping and gray
unmarking. The latter does not affect the result of the former, but any data
race is undefined behaviour.

This patch changes the marking bitsmaps to be relaxed atomic to avoid this.
Marking bitmaps were already excluded from TSAN checks.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85cff42af3a3
Make mark bitmaps atomic r=sfink

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/85cff42af3a3