op_switch_char broken for rope strings after JSRopeString layout rewrite
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Mar 2019 00:20:07 +0000 (00:20 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Mar 2019 00:20:07 +0000 (00:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195339
<rdar://problem/48592545>

Reviewed by Yusuke Suzuki.

JSTests:

* stress/switch-on-char-llint-rope.js: Added.

Source/JavaScriptCore:

When we did the JSString rewrite, we accidentally broke LLInt's switch_char
for rope strings. That change made it so that we always go to the slow path
for ropes. That's wrong. The slow path should only be taken when the rope
is of length 1. For lengths other than 1, we need to fall through to the
default case. This patch fixes this.

* llint/LowLevelInterpreter32_64.asm:
* llint/LowLevelInterpreter64.asm:
* runtime/JSString.h:

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

JSTests/ChangeLog
JSTests/stress/switch-on-char-llint-rope.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
Source/JavaScriptCore/runtime/JSString.h

index 1a49554..be003d3 100644 (file)
@@ -1,3 +1,13 @@
+2019-03-05  Saam barati  <sbarati@apple.com>
+
+        op_switch_char broken for rope strings after JSRopeString layout rewrite
+        https://bugs.webkit.org/show_bug.cgi?id=195339
+        <rdar://problem/48592545>
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/switch-on-char-llint-rope.js: Added.
+
 2019-03-04  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] Store bits for JSRopeString in 3 stores
diff --git a/JSTests/stress/switch-on-char-llint-rope.js b/JSTests/stress/switch-on-char-llint-rope.js
new file mode 100644 (file)
index 0000000..aff44b0
--- /dev/null
@@ -0,0 +1,22 @@
+function constStr() {
+    return ' ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ';
+}
+
+function foo(z) {
+    switch (z) {
+    case 'a':
+    case 'a':
+    case 'a':
+        return 1;
+    default:
+        return 2;
+    }
+}
+noInline(foo);
+
+let str = 'a' + constStr();
+for (let i = 0; i < 10000; ++i) {
+    let result = foo(str);
+    if (result !== 2)
+        throw new Error("Bad result");
+}
index 177ee6b..82c7335 100644 (file)
@@ -1,3 +1,21 @@
+2019-03-05  Saam barati  <sbarati@apple.com>
+
+        op_switch_char broken for rope strings after JSRopeString layout rewrite
+        https://bugs.webkit.org/show_bug.cgi?id=195339
+        <rdar://problem/48592545>
+
+        Reviewed by Yusuke Suzuki.
+
+        When we did the JSString rewrite, we accidentally broke LLInt's switch_char
+        for rope strings. That change made it so that we always go to the slow path
+        for ropes. That's wrong. The slow path should only be taken when the rope
+        is of length 1. For lengths other than 1, we need to fall through to the
+        default case. This patch fixes this.
+
+        * llint/LowLevelInterpreter32_64.asm:
+        * llint/LowLevelInterpreter64.asm:
+        * runtime/JSString.h:
+
 2019-03-05  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] Should check exception for JSString::toExistingAtomicString
index d7b1983..c77fb7e 100644 (file)
@@ -1788,15 +1788,15 @@ llintOpWithJump(op_switch_char, OpSwitchChar, macro (size, get, jump, dispatch)
     addp t3, t2
     bineq t1, CellTag, .opSwitchCharFallThrough
     bbneq JSCell::m_type[t0], StringType, .opSwitchCharFallThrough
-    loadp JSString::m_fiber[t0], t0
-    btpnz t0, isRopeInPointer, .opSwitchOnRope
-    bineq StringImpl::m_length[t0], 1, .opSwitchCharFallThrough
-    loadp StringImpl::m_data8[t0], t1
-    btinz StringImpl::m_hashAndFlags[t0], HashFlags8BitBuffer, .opSwitchChar8Bit
-    loadh [t1], t0
+    loadp JSString::m_fiber[t0], t1
+    btpnz t1, isRopeInPointer, .opSwitchOnRope
+    bineq StringImpl::m_length[t1], 1, .opSwitchCharFallThrough
+    loadp StringImpl::m_data8[t1], t0
+    btinz StringImpl::m_hashAndFlags[t1], HashFlags8BitBuffer, .opSwitchChar8Bit
+    loadh [t0], t0
     jmp .opSwitchCharReady
 .opSwitchChar8Bit:
-    loadb [t1], t0
+    loadb [t0], t0
 .opSwitchCharReady:
     subi SimpleJumpTable::min[t2], t0
     biaeq t0, SimpleJumpTable::branchOffsets + VectorSizeOffset[t2], .opSwitchCharFallThrough
@@ -1809,6 +1809,9 @@ llintOpWithJump(op_switch_char, OpSwitchChar, macro (size, get, jump, dispatch)
     jump(m_defaultOffset)
 
 .opSwitchOnRope:
+    bineq JSRopeString::m_compactFibers + JSRopeString::CompactFibers::m_length[t0], 1, .opSwitchCharFallThrough
+
+.opSwitchOnRopeChar:
     callSlowPath(_llint_slow_path_switch_char)
     nextInstruction()
 end)
index 42afc16..7961a7e 100644 (file)
@@ -1898,6 +1898,9 @@ llintOpWithJump(op_switch_char, OpSwitchChar, macro (size, get, jump, dispatch)
     jump(m_defaultOffset)
 
 .opSwitchOnRope:
+    bineq JSRopeString::m_compactFibers + JSRopeString::CompactFibers::m_length[t1], 1, .opSwitchCharFallThrough
+
+.opSwitchOnRopeChar:
     callSlowPath(_llint_slow_path_switch_char)
     nextInstruction()
 end)
index 2af1f03..e111f7e 100644 (file)
@@ -311,6 +311,8 @@ public:
         static ptrdiff_t offsetOfFiber2() { return OBJECT_OFFSETOF(CompactFibers, m_fiber1Upper); }
 
     private:
+        friend class LLIntOffsetsExtractor;
+
         uint32_t m_length { 0 };
         uint32_t m_fiber1Lower { 0 };
         uint16_t m_fiber1Upper { 0 };
@@ -350,6 +352,8 @@ public:
         static ptrdiff_t offsetOfFiber2() { return OBJECT_OFFSETOF(CompactFibers, m_fiber2); }
 
     private:
+        friend class LLIntOffsetsExtractor;
+
         uint32_t m_length { 0 };
         JSString* m_fiber1 { nullptr };
         JSString* m_fiber2 { nullptr };
@@ -438,6 +442,8 @@ public:
     }
 
 private:
+    friend class LLIntOffsetsExtractor;
+
     void convertToNonRope(String&&) const;
 
     void initializeIs8Bit(bool flag) const