DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeC...
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Aug 2019 06:49:28 +0000 (06:49 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Aug 2019 06:49:28 +0000 (06:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201281
<rdar://problem/54028228>

Reviewed by Yusuke Suzuki and Saam Barati.

JSTests:

* stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js: Added.

Source/JavaScriptCore:

This (see title above) is already the preferred idiom used in most places in our
compiler, except for 2: DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
compileStringCharAt().  Consider the following:

    bool prototypeChainIsSane = false;
    if (globalObject->stringPrototypeChainIsSane()) {
        ...
        m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
        m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));

        prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
    }

What's essential for correctness here is that the stringPrototype and objectPrototype
structures be loaded before the loads in the second stringPrototypeChainIsSane()
check.  Without a loadLoadFence before the second stringPrototypeChainIsSane()
check, we can't guarantee that.  Elsewhere in the compiler, the preferred idiom
for doing this right is to pre-load the structures first, do a loadLoadFence, and
then do the IsSane check just once after e.g.

    Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure(m_vm);
    Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_vm);

    if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
        && objectPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
        && globalObject->arrayPrototypeChainIsSane()) {

        m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
        m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
        ...
    }

This patch changes DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
compileStringCharAt() to follow the same idiom.

We also fix a bad assertion in Structure::storedPrototype() and
Structure::storedPrototypeObject().  The assertion is only correct when those
methods are called from the mutator thread.  The assertion has been updated to
only check its test condition if the current thread is the mutator thread.

* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileGetByValOnString):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):
* runtime/StructureInlines.h:
(JSC::Structure::storedPrototype const):
(JSC::Structure::storedPrototypeObject const):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@249247 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/runtime/StructureInlines.h

index c7a67f0..c5fe663 100644 (file)
@@ -1,5 +1,15 @@
 2019-08-28  Mark Lam  <mark.lam@apple.com>
 
+        DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.
+        https://bugs.webkit.org/show_bug.cgi?id=201281
+        <rdar://problem/54028228>
+
+        Reviewed by Yusuke Suzuki and Saam Barati.
+
+        * stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js: Added.
+
+2019-08-28  Mark Lam  <mark.lam@apple.com>
+
         Placate exception check validation in DFG's operationHasGenericProperty().
         https://bugs.webkit.org/show_bug.cgi?id=201245
         <rdar://problem/54777512>
diff --git a/JSTests/stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js b/JSTests/stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js
new file mode 100644 (file)
index 0000000..de76a05
--- /dev/null
@@ -0,0 +1,22 @@
+//@ skip if ["arm", "mips"].include?($architecture)
+//@ slow!
+//@ runDefault("--jitPolicyScale=0")
+
+// This test should not crash.
+
+// Increase iterations to 10000 if you want the regression to reproduce more reliably.
+// It can manifest in just a few iterations or may take a lot more iterations. We're
+// reducing iterations here to shorten the execution time of this test for normal runs,
+// with the tradeoff that some runs may not trigger the regression (if present). This is
+// so that fixed builds (which is the likely case going forward) won't have to wait too
+// long for this test to finish.
+const iterations = 500;
+for (let i = 0; i < iterations; i++) {
+    let code = `
+        for (let i = 0; i < 1000; i++) {
+            String.prototype.__proto__ = [];
+            const w = 'abcdefg'[-2];
+        }
+    `;
+    runString(code);
+}
index b0df4ec..d4b1cdb 100644 (file)
@@ -1,5 +1,61 @@
 2019-08-28  Mark Lam  <mark.lam@apple.com>
 
+        DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.
+        https://bugs.webkit.org/show_bug.cgi?id=201281
+        <rdar://problem/54028228>
+
+        Reviewed by Yusuke Suzuki and Saam Barati.
+
+        This (see title above) is already the preferred idiom used in most places in our
+        compiler, except for 2: DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
+        compileStringCharAt().  Consider the following:
+
+            bool prototypeChainIsSane = false;
+            if (globalObject->stringPrototypeChainIsSane()) {
+                ...
+                m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
+                m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));
+
+                prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
+            }
+
+        What's essential for correctness here is that the stringPrototype and objectPrototype
+        structures be loaded before the loads in the second stringPrototypeChainIsSane()
+        check.  Without a loadLoadFence before the second stringPrototypeChainIsSane()
+        check, we can't guarantee that.  Elsewhere in the compiler, the preferred idiom
+        for doing this right is to pre-load the structures first, do a loadLoadFence, and
+        then do the IsSane check just once after e.g.
+
+            Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure(m_vm);
+            Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_vm);
+
+            if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
+                && objectPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
+                && globalObject->arrayPrototypeChainIsSane()) {
+
+                m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
+                m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
+                ...
+            }
+
+        This patch changes DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
+        compileStringCharAt() to follow the same idiom.
+
+        We also fix a bad assertion in Structure::storedPrototype() and
+        Structure::storedPrototypeObject().  The assertion is only correct when those
+        methods are called from the mutator thread.  The assertion has been updated to
+        only check its test condition if the current thread is the mutator thread.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileGetByValOnString):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):
+        * runtime/StructureInlines.h:
+        (JSC::Structure::storedPrototype const):
+        (JSC::Structure::storedPrototypeObject const):
+
+2019-08-28  Mark Lam  <mark.lam@apple.com>
+
         Placate exception check validation in DFG's operationHasGenericProperty().
         https://bugs.webkit.org/show_bug.cgi?id=201245
         <rdar://problem/54777512>
index 1b2850b..cd54584 100644 (file)
@@ -2231,7 +2231,10 @@ void SpeculativeJIT::compileGetByValOnString(Node* node)
 #endif
 
         JSGlobalObject* globalObject = m_jit.globalObjectFor(node->origin.semantic);
-        bool prototypeChainIsSane = false;
+        Structure* stringPrototypeStructure = globalObject->stringPrototype()->structure(vm);
+        Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm);
+        WTF::loadLoadFence();
+
         if (globalObject->stringPrototypeChainIsSane()) {
             // FIXME: This could be captured using a Speculation mode that means "out-of-bounds
             // loads return a trivial value". Something like SaneChainOutOfBounds. This should
@@ -2239,11 +2242,9 @@ void SpeculativeJIT::compileGetByValOnString(Node* node)
             // on a stringPrototypeChainIsSane() guaranteeing that the prototypes have no negative
             // indexed properties either.
             // https://bugs.webkit.org/show_bug.cgi?id=144668
-            m_jit.graph().registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm));
-            m_jit.graph().registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm));
-            prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
-        }
-        if (prototypeChainIsSane) {
+            m_jit.graph().registerAndWatchStructureTransition(stringPrototypeStructure);
+            m_jit.graph().registerAndWatchStructureTransition(objectPrototypeStructure);
+
 #if USE(JSVALUE64)
             addSlowPathGenerator(makeUnique<SaneStringGetByValSlowPathGenerator>(
                 outOfBounds, this, JSValueRegs(scratchReg), baseReg, propertyReg));
index a70d5cd..7a89d62 100644 (file)
@@ -6978,20 +6978,19 @@ private:
             results.append(m_out.anchor(m_out.intPtrZero));
         } else {
             JSGlobalObject* globalObject = m_graph.globalObjectFor(m_node->origin.semantic);
-            
-            bool prototypeChainIsSane = false;
+            Structure* stringPrototypeStructure = globalObject->stringPrototype()->structure(vm());
+            Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm());
+            WTF::loadLoadFence();
+
             if (globalObject->stringPrototypeChainIsSane()) {
                 // FIXME: This could be captured using a Speculation mode that means
                 // "out-of-bounds loads return a trivial value", something like
                 // SaneChainOutOfBounds.
                 // https://bugs.webkit.org/show_bug.cgi?id=144668
                 
-                m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
-                m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));
+                m_graph.registerAndWatchStructureTransition(stringPrototypeStructure);
+                m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
 
-                prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
-            }
-            if (prototypeChainIsSane) {
                 LBasicBlock negativeIndex = m_out.newBlock();
                     
                 results.append(m_out.anchor(m_out.constInt64(JSValue::encode(jsUndefined()))));
index 5fab293..e02fba2 100644 (file)
@@ -108,7 +108,7 @@ inline Structure* Structure::storedPrototypeStructure() const
 
 ALWAYS_INLINE JSValue Structure::storedPrototype(const JSObject* object) const
 {
-    ASSERT(object->structure() == this);
+    ASSERT(!isMainThread() || object->structure() == this);
     if (hasMonoProto())
         return storedPrototype();
     return object->getDirect(knownPolyProtoOffset);
@@ -116,7 +116,7 @@ ALWAYS_INLINE JSValue Structure::storedPrototype(const JSObject* object) const
 
 ALWAYS_INLINE JSObject* Structure::storedPrototypeObject(const JSObject* object) const
 {
-    ASSERT(object->structure() == this);
+    ASSERT(!isMainThread() || object->structure() == this);
     if (hasMonoProto())
         return storedPrototypeObject();
     JSValue proto = object->getDirect(knownPolyProtoOffset);