1253461 - TSan: data race under compacting GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Just saw this when running jsapi-tests. Don't have time to look into it right this second.

testGCUID
==================
WARNING: ThreadSanitizer: data race (pid=30750)
  Write of size 8 at 0x7fb404f66018 by thread T11:
    #0 getNextArenaToUpdateAndUnlink /home/terrence/moz/branch/e/js/src/gc/Heap.h:653 (jsapi-tests+0x000000b5e58a)
    #1 updateArenas /home/terrence/moz/branch/e/js/src/jsgc.cpp:2638 (jsapi-tests+0x000000b5e58a)
    #2 run /home/terrence/moz/branch/e/js/src/jsgc.cpp:2650 (jsapi-tests+0x000000b5e627)
    #3 runFromHelperThread /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1083 (jsapi-tests+0x000000ccbbfb)
    #4 handleGCParallelWorkload /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1106 (jsapi-tests+0x000000cccd2e)
    #5 threadLoop /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1738 (jsapi-tests+0x000000cccd2e)
    #6 js::HelperThread::ThreadMain(void*) /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1345 (jsapi-tests+0x000000cca236)
    #7 ThreadRoutine /home/terrence/moz/branch/e/js/src/vm/PosixNSPR.cpp:45 (jsapi-tests+0x000000dc8a77)

  Previous read of size 8 at 0x7fb404f66018 by main thread:
    #0 getAllocKind /home/terrence/moz/branch/e/js/src/gc/Heap.h:550 (jsapi-tests+0x000000be24c8)
    #1 getAllocKind /home/terrence/moz/branch/e/js/src/gc/Heap.h:1227 (jsapi-tests+0x000000be24c8)
    #2 fixupDictionaryShapeAfterMovingGC /home/terrence/moz/branch/e/js/src/jspropertytree.cpp:242 (jsapi-tests+0x000000be24c8)
    #3 fixupAfterMovingGC /home/terrence/moz/branch/e/js/src/jspropertytree.cpp:307 (jsapi-tests+0x000000be24c8)
    #4 UpdateCellPointersTyped<js::Shape> /home/terrence/moz/branch/e/js/src/jsgc.cpp:2429 (jsapi-tests+0x000000b5e36c)
    #5 UpdateCellPointers /home/terrence/moz/branch/e/js/src/jsgc.cpp:2467 (jsapi-tests+0x000000b5e36c)
    #6 updateArenas /home/terrence/moz/branch/e/js/src/jsgc.cpp:2640 (jsapi-tests+0x000000b5e36c)
    #7 run /home/terrence/moz/branch/e/js/src/jsgc.cpp:2650 (jsapi-tests+0x000000b5e627)
    #8 js::GCParallelTask::runFromMainThread(JSRuntime*) /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:1071 (jsapi-tests+0x000000ccbb79)
    #9 updateAllCellPointersParallel /home/terrence/moz/branch/e/js/src/jsgc.cpp:2689 (jsapi-tests+0x000000b5ed01)
    #10 updatePointersToRelocatedCells /home/terrence/moz/branch/e/js/src/jsgc.cpp:2774 (jsapi-tests+0x000000b5f612)
    #11 compactPhase /home/terrence/moz/branch/e/js/src/jsgc.cpp:5675 (jsapi-tests+0x000000b720a7)
    #12 incrementalCollectSlice /home/terrence/moz/branch/e/js/src/jsgc.cpp:6143 (jsapi-tests+0x000000b73dc7)
    #13 gcCycle /home/terrence/moz/branch/e/js/src/jsgc.cpp:6318 (jsapi-tests+0x000000b74836)
    #14 collect /home/terrence/moz/branch/e/js/src/jsgc.cpp:6424 (jsapi-tests+0x000000b75212)
    #15 gc /home/terrence/moz/branch/e/js/src/jsgc.cpp:6482 (jsapi-tests+0x000000b78e85)
    #16 GCForReason /home/terrence/moz/branch/e/js/src/jsgc.cpp:7385 (jsapi-tests+0x000000b78e85)
    #17 run /home/terrence/moz/branch/e/js/src/jsapi-tests/testGCUniqueId.cpp:114 (jsapi-tests+0x000000588d2e)
    #18 main /home/terrence/moz/branch/e/js/src/jsapi-tests/tests.cpp:125 (jsapi-tests+0x00000064d206)

  Thread T11 'JS Helper' (tid=30762, running) created by main thread at:
    #0 pthread_create /home/terrence/moz/clang/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:954 (jsapi-tests+0x0000004606b6)
    #1 PR_CreateThread /home/terrence/moz/branch/e/js/src/vm/PosixNSPR.cpp:108 (jsapi-tests+0x000000dc8bc2)
    #2 ensureInitialized /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:607 (jsapi-tests+0x000000cc633c)
    #3 js::EnsureHelperThreadsInitialized() /home/terrence/moz/branch/e/js/src/vm/HelperThreads.cpp:59 (jsapi-tests+0x000000cc6212)
    #4 init /home/terrence/moz/branch/e/js/src/vm/Runtime.cpp:291 (jsapi-tests+0x000000dd9a68)
    #5 JS_NewRuntime /home/terrence/moz/branch/e/js/src/jsapi.cpp:471 (jsapi-tests+0x000000b0d8e6)
    #6 createRuntime /home/terrence/moz/branch/e/js/src/jsapi-tests/tests.h:283 (jsapi-tests+0x0000004e29e1)
    #7 init /home/terrence/moz/branch/e/js/src/jsapi-tests/tests.cpp:18 (jsapi-tests+0x00000064bd88)
    #8 main /home/terrence/moz/branch/e/js/src/jsapi-tests/tests.cpp:118 (jsapi-tests+0x00000064d1e3)

SUMMARY: ThreadSanitizer: data race /home/terrence/moz/branch/e/js/src/gc/Heap.h:653 in getNextArenaToUpdateAndUnlink
==================
TEST-PASS | testGCUID | ok

Comment 1

[Jon Coppeard (:jonco)]

This is because fixupDictionaryShapeAfterMovingGC is looking at the AllocKind in the arena header while a background thread is updating the linked list in auxNextLink.  These are separate fields but are all packed into the first 8 bytes of the arena header.

I don't think this is a 'real' race since the write will never change the alloc kind, but if we can do something less unpleasant in fixupDictionaryShapeAfterMovingGC that would be a win.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1253461-fix-shape-race

Pending try results...

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1253461-fix-shape-race v2

Try run is green.  This patch removes the checking the referent's arena header to tell what type of thing listp points to because we can tell by checking whether the base shape is owned.  As explained in the comments in Shape.h, the last shape in a dictionary mode list always has an owned base shape.

I kept the arena check in debug mode so we can assert that this is correct, but we could probably throw it away entirely.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8728347 [details] [diff] [review]
bug1253461-fix-shape-race v2

Review of attachment 8728347 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me. If we ever get our TSAN warnings down to near zero we can decide what to do to suppress this one in debug too.

::: js/src/jspropertytree.cpp
@@ +236,5 @@
>  
> +    // The listp field either points to the parent field of the next shape in
> +    // the list if there is one.  Otherwise if this shape is the last in the
> +    // list then it points to the shape_ field of the object the list is for.
> +    // We can tell which it is because the base shape is owend if this is the

owend => owned

@@ +238,5 @@
> +    // the list if there is one.  Otherwise if this shape is the last in the
> +    // list then it points to the shape_ field of the object the list is for.
> +    // We can tell which it is because the base shape is owend if this is the
> +    // last property and not otherwise.
> +    bool listpPointsIntoShape = !base()->isOwned();

\o/

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9aba4255d34

Comment 6

[Carsten Book [:Tomcat]]

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