Closed Bug 844759 Opened 7 years ago Closed 4 years ago
TSan: Thread data race in js::gc::Arena
Lists::allocate From Arena(JSCompartment*, js::gc::Alloc Kind) vs . js::gc::Arena Lists::background Finalize(js::Free Op*, js::gc::Arena Header*, bool)
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 with changeset 122820:c233837cce08. 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 .  http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
Summary: TSan: Thread data race in js::gc::ArenaLists::allocateFromArena(JSCompartment*, js::gc::AllocKind) → TSan: Thread data race in js::gc::ArenaLists::allocateFromArena(JSCompartment*, js::gc::AllocKind) vs. js::gc::ArenaLists::backgroundFinalize(js::FreeOp*, js::gc::ArenaHeader*, bool)
Ccing bhackett and billm for some help here :)
This is probably ArenaLists::backgroundFinalizeState, which is accessed with the GC lock held in backgroundFinalize and without in allocateFromArena - it's volatile and care is taken to ensure the main thread see changes from the background thread if necessary. So I think this is ok.
Same as for bug 844755 (comment 6), will put these on ignore when bug 847350 is through.
Assignee: nobody → choller
Depends on: 847350
Here's a patch for this one to use atomics rather than volatile for data shared between threads.
Decoder found that there is still a race reported with this patch applied. Edited IRC transcript: 11:38 decoder there is also one more, with maybegc on the stack, and it shows a load vs. a store from Atomics. i am not sure yet whats going on, because our atomics are compiler builtins recognized by tsan. maybe in this particular case something phishy i going on 11:38 decoder ill try to figure it out 11:41 decoder jonco: https://pastebin.mozilla.org/3366630 11:43 decoder jonco: thats with your patches applied btw 11:44 decoder jsgc.cpp line 2187 is: ArenaLists::backgroundFinalize(&fop, arenas, onBackgroundThread); 11:44 decoder and 1546 is: void *thing = cx->allocator()->arenas.allocateFromArenaInline(zone, thingKind); 11:45 jonco decoder: so it looks like the patch for 844759 didn't work 11:45 jonco decoder: I think it's that issue 11:46 decoder jonco: you mean the signature changed from that patch? 11:46 decoder i can try to confirm by backing that one out locally 11:59 decoder jonco: okay. without the patch im seeing two signatures there. the one you fixed with the patch and one load/store: https://pastebin.mozilla.org/3366716 12:00 decoder your patch certainly fixed one of these 12:00 decoder but the other one was still there or morphed into something else 12:00 decoder there could be two problems here, or the fix is incomplete
Even if this doesn't exactly fix the TASN errors, use of atomics is preferable to volatile.
Comment on attachment 8421032 [details] [diff] [review] bug844759-atomic-bfs Review of attachment 8421032 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Sadly, |volatile| is more a signal to the programmer that the memory is being used for synchronization than it is to the compiler.
Attachment #8421032 - Flags: review?(terrence) → review+
decoder, are you still seeing TSAN warnings relating to this?
Clearing old needinfo as I'm not working on TSan anymore. Nathan, since you've been running TSan in the past, do you still encounter this signature/issue?
Flags: needinfo?(choller) → needinfo?(nfroyd)
(In reply to Christian Holler (:decoder) from comment #11) > Clearing old needinfo as I'm not working on TSan anymore. > > Nathan, since you've been running TSan in the past, do you still encounter > this signature/issue? I haven't seen anything like this. I think this has been open long enough that a new bug is probably more appropriate in any event.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
You need to log in before you can comment on or make changes to this bug.