848650 - Helgrind warning about data race on 'rt->gcBytes'

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

==29510== Lock at 0x1295FDB0 was first observed
==29510==    at 0x4030466: pthread_mutex_init (hg_intercepts.c:429)
==29510==    by 0x408EEEE: PR_NewLock (ptsynch.c:152)
==29510==    by 0x8937E6B: js_InitGC(JSRuntime*, unsigned int) (jsgc.cpp:957)
==29510==    by 0x87EC4F9: JSRuntime::init(unsigned int) (jsapi.cpp:919)
==29510==    by 0x880D956: JS_NewRuntime(unsigned int, JSUseHelperThreads) (jsapi.cpp:1148)
==29510==    <snip>
==29510==    by 0x4056826: NS_InitXPCOM2 (nsXPComStub.cpp:127)
==29510==    by 0x40CC3E: main (xpcshell.cpp:1798)
==29510== 
==29510== Possible data race during read of size 8 at 0x12948D38 by thread #1
==29510== Locks held: none
==29510==    at 0x8D9D463: js::gcstats::Statistics::beginGC() (Statistics.cpp:531)
==29510==    by 0x8D9D784: js::gcstats::Statistics::beginSlice(int, int, int, JS::gcreason::Reason) (Statistics.cpp:582)
==29510==    by 0x893348D: js::gcstats::AutoGCSlice::AutoGCSlice(js::gcstats::Statistics&, int, int, int, JS::gcreason::Reason, mozilla::detail::GuardObjectNotifier const&) (Statistics.h:187)
==29510==    by 0x8949A94: Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, JS::gcreason::Reason) (jsgc.cpp:4486)
==29510==    by 0x8949D36: js::GC(JSRuntime*, js::JSGCInvocationKind, JS::gcreason::Reason) (jsgc.cpp:4523)
==29510==    by 0x8813DE9: JS_GC(JSRuntime*) (jsapi.cpp:2849)
==29510==    by 0x40D5FB: main (xpcshell.cpp:1955)
==29510== 
==29510== This conflicts with a previous write of size 8 by thread #5
==29510== Locks held: 1, at address 0x1295FDB0
==29510==    at 0x893779F: js::gc::Chunk::releaseArena(js::gc::ArenaHeader*) (jsgc.cpp:821)
==29510==    by 0x894C48E: bool FinalizeTypedArenas<JSString>(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) (jsgc.cpp:414)
==29510==    by 0x8935A6D: FinalizeArenas(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) (jsgc.cpp:459)
==29510==    by 0x8939D95: js::gc::ArenaLists::backgroundFinalize(js::FreeOp*, js::gc::ArenaHeader*, bool) (jsgc.cpp:1389)
==29510==    by 0x893D847: SweepBackgroundThings(JSRuntime*, bool) (jsgc.cpp:2201)
==29510==    by 0x893EE51: js::GCHelperThread::doSweep() (jsgc.cpp:2483)
==29510==    by 0x893DEEB: js::GCHelperThread::threadLoop() (jsgc.cpp:2327)
==29510==    by 0x893DE53: js::GCHelperThread::threadMain(void*) (jsgc.cpp:2306)
==29510==    by 0x409754C: _pt_root (ptthread.c:192)
==29510==    by 0x403032F: mythread_wrapper (hg_intercepts.c:219)
==29510==    by 0x4A2CE99: start_thread (pthread_create.c:308)
==29510==    by 0xB211CBC: clone (clone.S:112)

Basically the GC thread is writing 'rt->gcBytes' while holding the GC lock in 'Chunk::releaseArena()', and at the same time the main thread is reading the same location in 'Statistics::beginGC()'.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] from comment #0)
> and at the same time the main thread is reading the same
> location in 'Statistics::beginGC()'.

Reading without holding any lock, to be clear.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Bill says that this race is by design and should be harmless. Patch attached to disable helgrind's warning here.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Does it matter which access the annotation goes on? It would make more sense to me to put it on the one in releaseArena. We can say for sure that that's the only time the background thread accesses this field, while the main thread might access it in other places.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 723577 [details] [diff] [review]
Patch, v1

r+ with the annotation on the use in releaseArena. Thanks.

Comment 5

[Chris Peterson [:cpeterson]]

bent: did you forget to land this r+'d patch from March 2013? Is this Helgrind fix still relevant?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The patch that added the annotations never landed so this won't work as is. I forget why that effort stalled...

Comment 7

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.