Attachment 360035 Details for Bug 190693 – Patch

[patch] Patch

bug-190693-20190124135757.patch (text/plain), 8.71 KB, created by Yusuke Suzuki on 2019-01-24 13:57:58 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-24 13:57:58 PST

Size: 8.71 KB

patch

obsolete

>Subversion Revision: 240444
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index bdf15f7981394565e54f5cef9ceb8ee1c5d3ceff..cabf1476e72ae9a70c194c661041d7ba8f539278 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,40 @@
>+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 NOBODY (OOPS!).
>+
>+        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  Guillaume Emont  <guijemont@igalia.com>
> 
>         [JSC] Reenable baseline JIT on mips
>diff --git a/Source/JavaScriptCore/heap/ConservativeRoots.h b/Source/JavaScriptCore/heap/ConservativeRoots.h
>index 83e5ce4694267c3143f08b44b0a861df450ce1a6..3c064a9b777b06d049f4fa07042764f89f6bde6a 100644
>--- a/Source/JavaScriptCore/heap/ConservativeRoots.h
>+++ b/Source/JavaScriptCore/heap/ConservativeRoots.h
>@@ -42,7 +42,7 @@ class ConservativeRoots {
>     void add(void* begin, void* end, JITStubRoutineSet&, CodeBlockSet&);
>     
>     size_t size() const;
>-    HeapCell** roots();
>+    HeapCell** roots() const;
> 
> private:
>     static const size_t inlineCapacity = 128;
>@@ -68,7 +68,7 @@ inline size_t ConservativeRoots::size() const
>     return m_size;
> }
> 
>-inline HeapCell** ConservativeRoots::roots()
>+inline HeapCell** ConservativeRoots::roots() const
> {
>     return m_roots;
> }
>diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
>index d1667037ac5cdc9ab128613822e87dee3020f307..8da70dd04411a36242389b3f2a83a87bd94694de 100644
>--- a/Source/JavaScriptCore/heap/Heap.cpp
>+++ b/Source/JavaScriptCore/heap/Heap.cpp
>@@ -2621,15 +2621,22 @@ void Heap::addCoreConstraints()
>             TimingScope preConvergenceTimingScope(*this, "Constraint: conservative scan");
>             m_objectSpace.prepareForConservativeScan();
> 
>-            ConservativeRoots conservativeRoots(*this);
>-            SuperSamplerScope superSamplerScope(false);
>+            {
>+                ConservativeRoots conservativeRoots(*this);
>+                SuperSamplerScope superSamplerScope(false);
> 
>-            gatherStackRoots(conservativeRoots);
>-            gatherJSStackRoots(conservativeRoots);
>-            gatherScratchBufferRoots(conservativeRoots);
>+                gatherStackRoots(conservativeRoots);
>+                gatherJSStackRoots(conservativeRoots);
>+                gatherScratchBufferRoots(conservativeRoots);
> 
>-            SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::ConservativeScan);
>-            slotVisitor.append(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;
>         },
>@@ -2696,14 +2703,6 @@ void Heap::addCoreConstraints()
>         },
>         ConstraintVolatility::GreyedByExecution);
>     
>-    m_constraintSet->add(
>-        "Jsr", "JIT Stub Routines",
>-        [this] (SlotVisitor& slotVisitor) {
>-            SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::JITStubRoutines);
>-            m_jitStubRoutines->traceMarkedStubRoutines(slotVisitor);
>-        },
>-        ConstraintVolatility::GreyedByExecution);
>-    
>     m_constraintSet->add(
>         "Ws", "Weak Sets",
>         [this] (SlotVisitor& slotVisitor) {
>diff --git a/Source/JavaScriptCore/heap/SlotVisitor.cpp b/Source/JavaScriptCore/heap/SlotVisitor.cpp
>index 1d0bd10a9faa848e60254d7985335a33006e5365..59082966bf078db1b745b757e5cd33a17b977d85 100644
>--- a/Source/JavaScriptCore/heap/SlotVisitor.cpp
>+++ b/Source/JavaScriptCore/heap/SlotVisitor.cpp
>@@ -134,7 +134,7 @@ void SlotVisitor::clearMarkStacks()
>         });
> }
> 
>-void SlotVisitor::append(ConservativeRoots& conservativeRoots)
>+void SlotVisitor::append(const ConservativeRoots& conservativeRoots)
> {
>     HeapCell** roots = conservativeRoots.roots();
>     size_t size = conservativeRoots.size();
>diff --git a/Source/JavaScriptCore/heap/SlotVisitor.h b/Source/JavaScriptCore/heap/SlotVisitor.h
>index 0d2e6e0aa6fef9a96616dfcc8e511c91810b5476..77118d184ca8167c04666cc207802599ec4d384a 100644
>--- a/Source/JavaScriptCore/heap/SlotVisitor.h
>+++ b/Source/JavaScriptCore/heap/SlotVisitor.h
>@@ -86,7 +86,7 @@ class SlotVisitor {
>     const VM& vm() const;
>     Heap* heap() const;
> 
>-    void append(ConservativeRoots&);
>+    void append(const ConservativeRoots&);
>     
>     template<typename T, typename Traits> void append(const WriteBarrierBase<T, Traits>&);
>     template<typename T, typename Traits> void appendHidden(const WriteBarrierBase<T, Traits>&);
>diff --git a/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp b/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp
>index 2aa17f4173ea2f2b2ed483c6f2501248eea605ae..195ff02f397cc020701c5a10b6c30a63c70269d3 100644
>--- a/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp
>+++ b/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp
>@@ -43,8 +43,6 @@ namespace JSC {
> GCAwareJITStubRoutine::GCAwareJITStubRoutine(
>     const MacroAssemblerCodeRef<JITStubRoutinePtrTag>& code, VM& vm)
>     : JITStubRoutine(code)
>-    , m_mayBeExecuting(false)
>-    , m_isJettisoned(false)
> {
>     vm.heap.m_jitStubRoutines->add(this);
> }
>diff --git a/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h b/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h
>index f5d0f616369a40866604cf2585e8abe961ac5586..5dedc6ab8c9cf9886bb32f2195a735fc0774cbf4 100644
>--- a/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h
>+++ b/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h
>@@ -67,8 +67,8 @@ class GCAwareJITStubRoutine : public JITStubRoutine {
> private:
>     friend class JITStubRoutineSet;
> 
>-    bool m_mayBeExecuting;
>-    bool m_isJettisoned;
>+    bool m_mayBeExecuting { false };
>+    bool m_isJettisoned { false };
> };
> 
> // Use this if you want to mark one additional object during GC if your stub
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index e4819f84e0e288f4ee8fc649fd7764d8969d27b2..e2838988aad4a51d34d1dd844c07801eb622cb7d 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,16 @@
>+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 NOBODY (OOPS!).
>+
>+        * stress/regress-190693.js: Added.
>+        (truth):
>+        (assert):
>+        (shouldThrowInvalidConstAssignment):
>+        (taz):
>+
> 2019-01-24  Guillaume Emont  <guijemont@igalia.com>
> 
>         [JSC] Reenable baseline JIT on mips

Flags:

msaboff: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 190693: 360035