Attachment 362432 Details for Bug 193938 – proposed patch.

[patch] proposed patch.

bug-193938.patch (text/plain), 14.77 KB, created by Mark Lam on 2019-02-19 14:39:01 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-02-19 14:39:01 PST

Size: 14.77 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 241772)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,61 @@
>+2019-02-19  Mark Lam  <mark.lam@apple.com>
>+
>+        Add code to validate expected GC activity modelled by doesGC() against what the runtime encounters.
>+        https://bugs.webkit.org/show_bug.cgi?id=193938
>+        <rdar://problem/47616277>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In DFG::SpeculativeJIT::compile() and FTL::LowerDFGToB3::compileNode(), before
>+        emitting code / B3IR for each DFG node, we emit a write to set Heap::m_expectDoesGC
>+        to the value returned by doesGC() for that node.  In the runtime (i.e. in allocateCell()
>+        and functions that can resolve a rope), we assert that Heap::m_expectDoesGC is
>+        true.
>+
>+        This validation code is currently only enabled for debug builds.  It is disabled
>+        for release builds by default, but it can easily be made to run on release builds
>+        as well by forcing ENABLE_DFG_DOES_GC_VALIDATION to 1 in Heap.h.
>+
>+        To allow this validation code to run on release builds as well, the validation uses
>+        RELEASE_ASSERT instead of ASSERT.
>+
>+        To ensure that Heap.h is #include'd for all files that needs to so this validation
>+        (so that the validation code is accidentally disabled), we guard the validation
>+        code with an if conditional on constexpr bool validateDFGDoesGC (instead of using
>+        a #if ENABLE(DFG_DOES_GC_VALIDATION)).  This way, if Heap.h isn't #include'd, the
>+        validation code will fail to build (no silent failures).
>+
>+        Currently, all JSC tests and Layout tests should pass with this validation enabled
>+        in debug builds.  We'll only see new failures if there's a regression or if new
>+        tests reveal a previously untested code path that has an undetected issue.
>+
>+        * dfg/DFGOSRExit.cpp:
>+        (JSC::DFG::OSRExit::executeOSRExit):
>+        (JSC::DFG::OSRExit::compileExit):
>+        * dfg/DFGSpeculativeJIT64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+        * ftl/FTLLowerDFGToB3.cpp:
>+        (JSC::FTL::DFG::LowerDFGToB3::compileNode):
>+        * ftl/FTLOSRExitCompiler.cpp:
>+        (JSC::FTL::compileStub):
>+        * heap/Heap.h:
>+        (JSC::Heap::expectDoesGC const):
>+        (JSC::Heap::setExpectDoesGC):
>+        (JSC::Heap::addressOfExpectDoesGC):
>+        * jit/JITArithmetic.cpp:
>+        (JSC::JIT::emit_compareAndJump):
>+        * runtime/JSCellInlines.h:
>+        (JSC::tryAllocateCellHelper):
>+        * runtime/JSString.h:
>+        (JSC::jsSingleCharacterString):
>+        (JSC::JSString::toAtomicString const):
>+        (JSC::JSString::toExistingAtomicString const):
>+        (JSC::JSString::value const):
>+        (JSC::JSString::tryGetValue const):
>+        (JSC::JSRopeString::unsafeView const):
>+        (JSC::JSRopeString::viewWithUnderlyingString const):
>+        (JSC::JSString::unsafeView const):
>+
> 2019-02-19  Mark Lam  <mark.lam@apple.com>
> 
>         Fix DFG doesGC() for TryGetById and ProfileType nodes.
>Index: Source/JavaScriptCore/dfg/DFGOSRExit.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGOSRExit.cpp	(revision 241772)
>+++ Source/JavaScriptCore/dfg/DFGOSRExit.cpp	(working copy)
>@@ -338,6 +338,12 @@ void OSRExit::executeOSRExit(Context& co
>     ASSERT(&exec->vm() == &vm);
>     auto& cpu = context.cpu;
> 
>+    if (validateDFGDoesGC) {
>+        // We're about to exit optimized code. So, there's no longer any optimized
>+        // code running that expects no GC.
>+        vm.heap.setExpectDoesGC(true);
>+    }
>+
>     if (vm.callFrameForCatch) {
>         exec = vm.callFrameForCatch;
>         context.fp() = exec;
>@@ -1389,6 +1395,12 @@ void OSRExit::compileExit(CCallHelpers& 
>         }
>     }
> 
>+    if (validateDFGDoesGC) {
>+        // We're about to exit optimized code. So, there's no longer any optimized
>+        // code running that expects no GC.
>+        jit.store8(CCallHelpers::TrustedImm32(true), vm.heap.addressOfExpectDoesGC());
>+    }
>+
>     // Need to ensure that the stack pointer accounts for the worst-case stack usage at exit. This
>     // could toast some stack that the DFG used. We need to do it before storing to stack offsets
>     // used by baseline.
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(revision 241772)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(working copy)
>@@ -33,6 +33,7 @@
> #include "CallFrameShuffler.h"
> #include "DFGAbstractInterpreterInlines.h"
> #include "DFGCallArrayAllocatorSlowPathGenerator.h"
>+#include "DFGDoesGC.h"
> #include "DFGOperations.h"
> #include "DFGSlowPathGenerator.h"
> #include "DirectArguments.h"
>@@ -1899,7 +1900,12 @@ void SpeculativeJIT::emitBranch(Node* no
> void SpeculativeJIT::compile(Node* node)
> {
>     NodeType op = node->op();
>-    
>+
>+    if (validateDFGDoesGC) {
>+        bool expectDoesGC = doesGC(m_jit.graph(), node);
>+        m_jit.store8(TrustedImm32(expectDoesGC), m_jit.vm()->heap.addressOfExpectDoesGC());
>+    }
>+
> #if ENABLE(DFG_REGISTER_ALLOCATION_VALIDATION)
>     m_jit.clearRegisterAllocationOffsets();
> #endif
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(revision 241772)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(working copy)
>@@ -43,6 +43,7 @@
> #include "CodeBlockWithJITType.h"
> #include "DFGAbstractInterpreterInlines.h"
> #include "DFGCapabilities.h"
>+#include "DFGDoesGC.h"
> #include "DFGDominators.h"
> #include "DFGInPlaceAbstractState.h"
> #include "DFGMayExit.h"
>@@ -525,7 +526,12 @@ private:
>         
>         m_interpreter.startExecuting();
>         m_interpreter.executeKnownEdgeTypes(m_node);
>-        
>+
>+        if (validateDFGDoesGC) {
>+            bool expectDoesGC = doesGC(m_graph, m_node);
>+            m_out.store(m_out.constBool(expectDoesGC), m_out.absolute(vm().heap.addressOfExpectDoesGC()));
>+        }
>+
>         switch (m_node->op()) {
>         case DFG::Upsilon:
>             compileUpsilon();
>Index: Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp	(revision 241772)
>+++ Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp	(working copy)
>@@ -244,6 +244,12 @@ static void compileStub(
> 
>     saveAllRegisters(jit, registerScratch);
>     
>+    if (validateDFGDoesGC) {
>+        // We're about to exit optimized code. So, there's no longer any optimized
>+        // code running that expects no GC.
>+        jit.store8(CCallHelpers::TrustedImm32(true), vm->heap.addressOfExpectDoesGC());
>+    }
>+
>     // Bring the stack back into a sane form and assert that it's sane.
>     jit.popToRestore(GPRInfo::regT0);
>     jit.checkStackPointerAlignment();
>Index: Source/JavaScriptCore/heap/Heap.h
>===================================================================
>--- Source/JavaScriptCore/heap/Heap.h	(revision 241772)
>+++ Source/JavaScriptCore/heap/Heap.h	(working copy)
>@@ -95,6 +95,13 @@ class SpeculativeJIT;
> class Worklist;
> }
> 
>+#if !ASSERT_DISABLED
>+#define ENABLE_DFG_DOES_GC_VALIDATION 1
>+#else
>+#define ENABLE_DFG_DOES_GC_VALIDATION 0
>+#endif
>+constexpr bool validateDFGDoesGC = ENABLE_DFG_DOES_GC_VALIDATION;
>+
> typedef HashCountedSet<JSCell*> ProtectCountSet;
> typedef HashCountedSet<const char*> TypeCountSet;
> 
>@@ -294,6 +301,16 @@ public:
>     unsigned barrierThreshold() const { return m_barrierThreshold; }
>     const unsigned* addressOfBarrierThreshold() const { return &m_barrierThreshold; }
> 
>+#if ENABLE(DFG_DOES_GC_VALIDATION)
>+    bool expectDoesGC() const { return m_expectDoesGC; }
>+    void setExpectDoesGC(bool value) { m_expectDoesGC = value; }
>+    bool* addressOfExpectDoesGC() { return &m_expectDoesGC; }
>+#else
>+    bool expectDoesGC() const { return true; }
>+    void setExpectDoesGC(bool) { }
>+    bool* addressOfExpectDoesGC() { return nullptr; }
>+#endif
>+
>     // If true, the GC believes that the mutator is currently messing with the heap. We call this
>     // "having heap access". The GC may block if the mutator is in this state. If false, the GC may
>     // currently be doing things to the heap that make the heap unsafe to access for the mutator.
>@@ -581,6 +598,9 @@ private:
>     Markable<CollectionScope, EnumMarkableTraits<CollectionScope>> m_collectionScope;
>     Markable<CollectionScope, EnumMarkableTraits<CollectionScope>> m_lastCollectionScope;
>     Lock m_raceMarkStackLock;
>+#if ENABLE(DFG_DOES_GC_VALIDATION)
>+    bool m_expectDoesGC { true };
>+#endif
> 
>     StructureIDTable m_structureIDTable;
>     MarkedSpace m_objectSpace;
>Index: Source/JavaScriptCore/jit/JITArithmetic.cpp
>===================================================================
>--- Source/JavaScriptCore/jit/JITArithmetic.cpp	(revision 241772)
>+++ Source/JavaScriptCore/jit/JITArithmetic.cpp	(working copy)
>@@ -179,13 +179,14 @@ void JIT::emit_compareAndJump(const Inst
>     int op1 = bytecode.m_lhs.offset();
>     int op2 = bytecode.m_rhs.offset();
>     unsigned target = jumpTarget(instruction, bytecode.m_targetLabel);
>+    bool disallowAllocation = false;
>     if (isOperandConstantChar(op1)) {
>         emitGetVirtualRegister(op2, regT0);
>         addSlowCase(branchIfNotCell(regT0));
>         JumpList failures;
>         emitLoadCharacterString(regT0, regT0, failures);
>         addSlowCase(failures);
>-        addJump(branch32(commute(condition), regT0, Imm32(asString(getConstantOperand(op1))->tryGetValue()[0])), target);
>+        addJump(branch32(commute(condition), regT0, Imm32(asString(getConstantOperand(op1))->tryGetValue(disallowAllocation)[0])), target);
>         return;
>     }
>     if (isOperandConstantChar(op2)) {
>@@ -194,7 +195,7 @@ void JIT::emit_compareAndJump(const Inst
>         JumpList failures;
>         emitLoadCharacterString(regT0, regT0, failures);
>         addSlowCase(failures);
>-        addJump(branch32(condition, regT0, Imm32(asString(getConstantOperand(op2))->tryGetValue()[0])), target);
>+        addJump(branch32(condition, regT0, Imm32(asString(getConstantOperand(op2))->tryGetValue(disallowAllocation)[0])), target);
>         return;
>     }
>     if (isOperandConstantInt(op2)) {
>Index: Source/JavaScriptCore/runtime/JSCellInlines.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSCellInlines.h	(revision 241772)
>+++ Source/JavaScriptCore/runtime/JSCellInlines.h	(working copy)
>@@ -166,6 +166,9 @@ template<typename T>
> ALWAYS_INLINE void* tryAllocateCellHelper(Heap& heap, size_t size, GCDeferralContext* deferralContext, AllocationFailureMode failureMode)
> {
>     VM& vm = *heap.vm();
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(heap.expectDoesGC());
>+
>     ASSERT(deferralContext || !DisallowGC::isInEffectOnCurrentThread());
>     ASSERT(size >= sizeof(T));
>     JSCell* result = static_cast<JSCell*>(subspaceFor<T>(vm)->allocateNonVirtual(vm, size, deferralContext, failureMode));
>Index: Source/JavaScriptCore/runtime/JSString.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSString.h	(revision 241772)
>+++ Source/JavaScriptCore/runtime/JSString.h	(working copy)
>@@ -163,7 +163,7 @@ public:
> 
>     inline bool equal(ExecState*, JSString* other) const;
>     const String& value(ExecState*) const;
>-    inline const String& tryGetValue() const;
>+    inline const String& tryGetValue(bool allocationAllowed = true) const;
>     const StringImpl* tryGetValueImpl() const;
>     ALWAYS_INLINE unsigned length() const { return m_length; }
> 
>@@ -515,6 +515,8 @@ inline JSString* jsEmptyString(VM* vm)
> 
> ALWAYS_INLINE JSString* jsSingleCharacterString(VM* vm, UChar c)
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(vm->heap.expectDoesGC());
>     if (c <= maxSingleCharacterString)
>         return vm->smallStrings.singleCharacterString(c);
>     return JSString::create(*vm, StringImpl::create(&c, 1));
>@@ -539,6 +541,8 @@ ALWAYS_INLINE Identifier JSString::toIde
> 
> ALWAYS_INLINE AtomicString JSString::toAtomicString(ExecState* exec) const
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(vm()->heap.expectDoesGC());
>     if (isRope())
>         static_cast<const JSRopeString*>(this)->resolveRopeToAtomicString(exec);
>     return AtomicString(m_value);
>@@ -546,6 +550,8 @@ ALWAYS_INLINE AtomicString JSString::toA
> 
> ALWAYS_INLINE RefPtr<AtomicStringImpl> JSString::toExistingAtomicString(ExecState* exec) const
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(vm()->heap.expectDoesGC());
>     if (isRope())
>         return static_cast<const JSRopeString*>(this)->resolveRopeToExistingAtomicString(exec);
>     if (m_value.impl()->isAtomic())
>@@ -555,17 +561,24 @@ ALWAYS_INLINE RefPtr<AtomicStringImpl> J
> 
> inline const String& JSString::value(ExecState* exec) const
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(vm()->heap.expectDoesGC());
>     if (isRope())
>         static_cast<const JSRopeString*>(this)->resolveRope(exec);
>     return m_value;
> }
> 
>-inline const String& JSString::tryGetValue() const
>+inline const String& JSString::tryGetValue(bool allocationAllowed) const
> {
>-    if (isRope()) {
>-        // Pass nullptr for the ExecState so that resolveRope does not throw in the event of an OOM error.
>-        static_cast<const JSRopeString*>(this)->resolveRope(nullptr);
>-    }
>+    if (allocationAllowed) {
>+        if (validateDFGDoesGC)
>+            RELEASE_ASSERT(vm()->heap.expectDoesGC());
>+        if (isRope()) {
>+            // Pass nullptr for the ExecState so that resolveRope does not throw in the event of an OOM error.
>+            static_cast<const JSRopeString*>(this)->resolveRope(nullptr);
>+        }
>+    } else
>+        RELEASE_ASSERT(!isRope());
>     return m_value;
> }
> 
>@@ -739,6 +752,8 @@ inline bool isJSString(JSValue v)
> 
> ALWAYS_INLINE StringView JSRopeString::unsafeView(ExecState* exec) const
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(vm()->heap.expectDoesGC());
>     if (isSubstring()) {
>         if (is8Bit())
>             return StringView(substringBase()->m_value.characters8() + substringOffset(), length());
>@@ -750,6 +765,8 @@ ALWAYS_INLINE StringView JSRopeString::u
> 
> ALWAYS_INLINE StringViewWithUnderlyingString JSRopeString::viewWithUnderlyingString(ExecState* exec) const
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(vm()->heap.expectDoesGC());
>     if (isSubstring()) {
>         auto& base = substringBase()->m_value;
>         if (is8Bit())
>@@ -762,6 +779,8 @@ ALWAYS_INLINE StringViewWithUnderlyingSt
> 
> ALWAYS_INLINE StringView JSString::unsafeView(ExecState* exec) const
> {
>+    if (validateDFGDoesGC)
>+        RELEASE_ASSERT(vm()->heap.expectDoesGC());
>     if (isRope())
>         return static_cast<const JSRopeString*>(this)->unsafeView(exec);
>     return m_value;

Flags:

msaboff: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 193938: 362432 | 362457 | 362559