[JSC] isRope jump in StringSlice should not jump over register allocations
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Apr 2019 00:00:24 +0000 (00:00 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Apr 2019 00:00:24 +0000 (00:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196716

Reviewed by Saam Barati.

JSTests:

* stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js: Added.
(foo.bar):
(foo):

Source/JavaScriptCore:

Jumping over the register allocation code in DFG (like the following) is wrong.

    auto jump = m_jit.branchXXX();
    {
        GPRTemporary reg(this);
        GPRReg regGPR = reg.gpr();
        ...
    }
    jump.link(&m_jit);

When GPRTemporary::gpr allocates a new register, it can flush the previous register value into the stack and make the register usable.
Jumping over this register allocation code skips the flushing code, and makes the DFG's stack and register content tracking inconsistent:
DFG thinks that the content is flushed and stored in particular stack slot even while this flushing code is skipped.
In this patch, we perform register allocations before jumping to the slow path based on `isRope` condition in StringSlice.

* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileStringSlice):

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

JSTests/ChangeLog
JSTests/stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

index 98cb0d1..0b4d69f 100644 (file)
@@ -1,5 +1,16 @@
 2019-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] isRope jump in StringSlice should not jump over register allocations
+        https://bugs.webkit.org/show_bug.cgi?id=196716
+
+        Reviewed by Saam Barati.
+
+        * stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js: Added.
+        (foo.bar):
+        (foo):
+
+2019-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
+
         [JSC] to_index_string should not assume incoming value is Uint32
         https://bugs.webkit.org/show_bug.cgi?id=196713
 
diff --git a/JSTests/stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js b/JSTests/stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js
new file mode 100644 (file)
index 0000000..198e8cb
--- /dev/null
@@ -0,0 +1,17 @@
+//@ runDefault("--jitPolicyScale=0", "--useFTLJIT=0", "--useConcurrentJIT=0", "--collectContinuously=1")
+
+function foo() {
+    for (let i = 0; i < 400; i++) {
+        const s = 'a'+'a';
+        function bar() {
+            s.slice(0);
+        }
+        for (const _ in {}) {
+        }
+        const o = {
+        };
+        bar([], [], [], [], {});
+        o ** '';
+    }
+}
+foo();
index 637e3a9..e61ca26 100644 (file)
@@ -1,5 +1,30 @@
 2019-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] isRope jump in StringSlice should not jump over register allocations
+        https://bugs.webkit.org/show_bug.cgi?id=196716
+
+        Reviewed by Saam Barati.
+
+        Jumping over the register allocation code in DFG (like the following) is wrong.
+
+            auto jump = m_jit.branchXXX();
+            {
+                GPRTemporary reg(this);
+                GPRReg regGPR = reg.gpr();
+                ...
+            }
+            jump.link(&m_jit);
+
+        When GPRTemporary::gpr allocates a new register, it can flush the previous register value into the stack and make the register usable.
+        Jumping over this register allocation code skips the flushing code, and makes the DFG's stack and register content tracking inconsistent:
+        DFG thinks that the content is flushed and stored in particular stack slot even while this flushing code is skipped.
+        In this patch, we perform register allocations before jumping to the slow path based on `isRope` condition in StringSlice.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileStringSlice):
+
+2019-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
+
         [JSC] to_index_string should not assume incoming value is Uint32
         https://bugs.webkit.org/show_bug.cgi?id=196713
 
index 036cb92..15ab44b 100644 (file)
@@ -1547,16 +1547,15 @@ void SpeculativeJIT::compileStringSlice(Node* node)
     }
 
     GPRTemporary temp(this);
-    GPRReg tempGPR = temp.gpr();
-
-    m_jit.loadPtr(CCallHelpers::Address(stringGPR, JSString::offsetOfValue()), tempGPR);
-    auto isRope = m_jit.branchIfRopeStringImpl(tempGPR);
-
     GPRTemporary temp2(this);
     GPRTemporary startIndex(this);
 
+    GPRReg tempGPR = temp.gpr();
     GPRReg temp2GPR = temp2.gpr();
     GPRReg startIndexGPR = startIndex.gpr();
+
+    m_jit.loadPtr(CCallHelpers::Address(stringGPR, JSString::offsetOfValue()), tempGPR);
+    auto isRope = m_jit.branchIfRopeStringImpl(tempGPR);
     {
         m_jit.load32(MacroAssembler::Address(tempGPR, StringImpl::lengthMemoryOffset()), temp2GPR);