Changeset 240449 in webkit

Timestamp:

Jan 24, 2019 2:37:01 PM (5 years ago)

Author:

ysuzuki@apple.com

Message:

stress/const-semantics.js fails a dfg-eager / ftl-eager run with an ASAN release build.
​https://bugs.webkit.org/show_bug.cgi?id=190693

Reviewed by Michael Saboff.

JSTests:

• stress/regress-190693.js: Added.

(truth):
(assert):
(shouldThrowInvalidConstAssignment):
(taz):

Source/JavaScriptCore:

JITStubRoutine's fields are marked only when JITStubRoutine::m_mayBeExecuting is true.
This becomes true when we find the executable address in our conservative roots, which
means that we could be executing it right now. This means that object liveness in
JITStubRoutine depends on the information gathered in ConservativeRoots. However, our
constraints are separated, "Conservative Scan" and "JIT Stub Routines". They can even
be executed concurrently, so that "JIT Stub Routines" may miss to mark the actually
executing JITStubRoutine because "Conservative Scan" finds it later.
When finalizing the GC, we delete the dead JITStubRoutines. At that time, since
"Conservative Scan" already finishes, we do not delete some JITStubRoutines which do not
mark the depending objects. Then, in the next cycle, we find JITStubRoutines still live,
attempt to mark the depending objects, and encounter the dead objects which are collected
in the previous cycles.

This patch removes "JIT Stub Routines" and merge it to "Conservative Scan". Since
"Conservative Scan" and "JIT Stub Routines" need to be executed only when the execution
happens (ensured by GreyedByExecution and CollectionPhase check), this change is OK for
GC stop time.

• heap/ConservativeRoots.h:

(JSC::ConservativeRoots::roots const):
(JSC::ConservativeRoots::roots): Deleted.

• heap/Heap.cpp:

(JSC::Heap::addCoreConstraints):

• heap/SlotVisitor.cpp:

(JSC::SlotVisitor::append):

• heap/SlotVisitor.h:
• jit/GCAwareJITStubRoutine.cpp:

(JSC::GCAwareJITStubRoutine::GCAwareJITStubRoutine):

• jit/GCAwareJITStubRoutine.h:

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/regress-190693.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/heap/ConservativeRoots.h (modified) (2 diffs)
• Source/JavaScriptCore/heap/Heap.cpp (modified) (2 diffs)
• Source/JavaScriptCore/heap/SlotVisitor.cpp (modified) (1 diff)
• Source/JavaScriptCore/heap/SlotVisitor.h (modified) (1 diff)
• Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp (modified) (1 diff)
• Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-01-24  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        stress/const-semantics.js fails a dfg-eager / ftl-eager run with an ASAN release build.
+        https://bugs.webkit.org/show_bug.cgi?id=190693
+
+        Reviewed by Michael Saboff.
+
+        * stress/regress-190693.js: Added.
+        (truth):
+        (assert):
+        (shouldThrowInvalidConstAssignment):
+        (taz):
+
 2019-01-24  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-01-24  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        stress/const-semantics.js fails a dfg-eager / ftl-eager run with an ASAN release build.
+        https://bugs.webkit.org/show_bug.cgi?id=190693
+
+        Reviewed by Michael Saboff.
+
+        JITStubRoutine's fields are marked only when JITStubRoutine::m_mayBeExecuting is true.
+        This becomes true when we find the executable address in our conservative roots, which
+        means that we could be executing it right now. This means that object liveness in
+        JITStubRoutine depends on the information gathered in ConservativeRoots. However, our
+        constraints are separated, "Conservative Scan" and "JIT Stub Routines". They can even
+        be executed concurrently, so that "JIT Stub Routines" may miss to mark the actually
+        executing JITStubRoutine because "Conservative Scan" finds it later.
+        When finalizing the GC, we delete the dead JITStubRoutines. At that time, since
+        "Conservative Scan" already finishes, we do not delete some JITStubRoutines which do not
+        mark the depending objects. Then, in the next cycle, we find JITStubRoutines still live,
+        attempt to mark the depending objects, and encounter the dead objects which are collected
+        in the previous cycles.
+
+        This patch removes "JIT Stub Routines" and merge it to "Conservative Scan". Since
+        "Conservative Scan" and "JIT Stub Routines" need to be executed only when the execution
+        happens (ensured by GreyedByExecution and CollectionPhase check), this change is OK for
+        GC stop time.
+
+        * heap/ConservativeRoots.h:
+        (JSC::ConservativeRoots::roots const):
+        (JSC::ConservativeRoots::roots): Deleted.
+        * heap/Heap.cpp:
+        (JSC::Heap::addCoreConstraints):
+        * heap/SlotVisitor.cpp:
+        (JSC::SlotVisitor::append):
+        * heap/SlotVisitor.h:
+        * jit/GCAwareJITStubRoutine.cpp:
+        (JSC::GCAwareJITStubRoutine::GCAwareJITStubRoutine):
+        * jit/GCAwareJITStubRoutine.h:
+
 2019-01-24  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/heap/ConservativeRoots.h

@@ -43 +43 @@
     
     size_t size() const;
-    HeapCell** roots();
+    HeapCell** roots() const;
 
 private:
@@ -69 +69 @@
 }
 
-inline HeapCell** ConservativeRoots::roots()
+inline HeapCell** ConservativeRoots::roots() const
 {
     return m_roots;

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -2622 +2622 @@
             m_objectSpace.prepareForConservativeScan();
 
-            ConservativeRoots conservativeRoots(*this);
-            SuperSamplerScope superSamplerScope(false);
-
-            gatherStackRoots(conservativeRoots);
-            gatherJSStackRoots(conservativeRoots);
-            gatherScratchBufferRoots(conservativeRoots);
-
-            SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::ConservativeScan);
-            slotVisitor.append(conservativeRoots);
+            {
+                ConservativeRoots conservativeRoots(*this);
+                SuperSamplerScope superSamplerScope(false);
+
+                gatherStackRoots(conservativeRoots);
+                gatherJSStackRoots(conservativeRoots);
+                gatherScratchBufferRoots(conservativeRoots);
+
+                SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::ConservativeScan);
+                slotVisitor.append(conservativeRoots);
+            }
+            {
+                // JITStubRoutines must be visited after scanning ConservativeRoots since JITStubRoutines depend on the hook executed during gathering ConservativeRoots.
+                SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::JITStubRoutines);
+                m_jitStubRoutines->traceMarkedStubRoutines(slotVisitor);
+            }
             
             lastVersion = m_phaseVersion;
@@ -2694 +2701 @@
             
             m_vm->shadowChicken().visitChildren(slotVisitor);
-        },
-        ConstraintVolatility::GreyedByExecution);
-    
-    m_constraintSet->add(
-        "Jsr", "JIT Stub Routines",
-        [this] (SlotVisitor& slotVisitor) {
-            SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::JITStubRoutines);
-            m_jitStubRoutines->traceMarkedStubRoutines(slotVisitor);
         },
         ConstraintVolatility::GreyedByExecution);

trunk/Source/JavaScriptCore/heap/SlotVisitor.cpp

@@ -135 +135 @@
 }
 
-void SlotVisitor::append(ConservativeRoots& conservativeRoots)
+void SlotVisitor::append(const ConservativeRoots& conservativeRoots)
 {
     HeapCell** roots = conservativeRoots.roots();

trunk/Source/JavaScriptCore/heap/SlotVisitor.h

@@ -87 +87 @@
     Heap* heap() const;
 
-    void append(ConservativeRoots&);
+    void append(const ConservativeRoots&);
     
     template<typename T, typename Traits> void append(const WriteBarrierBase<T, Traits>&);

trunk/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp

@@ -44 +44 @@
     const MacroAssemblerCodeRef<JITStubRoutinePtrTag>& code, VM& vm)
     : JITStubRoutine(code)
-    , m_mayBeExecuting(false)
-    , m_isJettisoned(false)
 {
     vm.heap.m_jitStubRoutines->add(this);

trunk/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h

@@ -68 +68 @@
     friend class JITStubRoutineSet;
 
-    bool m_mayBeExecuting;
-    bool m_isJettisoned;
+    bool m_mayBeExecuting { false };
+    bool m_isJettisoned { false };
 };