Attachment 369895 Details for Bug 197797 – WIP - Patch

[patch] WIP - Patch

armv7_tail_calls.diff (text/plain), 5.51 KB, created by Caio Lima on 2019-05-14 14:37:01 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Caio Lima

Created: 2019-05-14 14:37:01 PDT

Size: 5.51 KB

patch

obsolete

>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 2c83cfcdff0..252889c15b9 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2019-05-10  Caio Lima  <ticaiolima@gmail.com>
>+
>+        [ARMv7] Tail calls are broken on THUMB2
>+        https://bugs.webkit.org/show_bug.cgi?id=197797
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/tail-call-with-spilled-registers.js: Added.
>+
> 2019-05-10  Saam barati  <sbarati@apple.com>
> 
>         Call to JSToWasmICCallee::createStructure passes in wrong prototype value
>diff --git a/JSTests/stress/tail-call-with-spilled-registers.js b/JSTests/stress/tail-call-with-spilled-registers.js
>new file mode 100644
>index 00000000000..03fb71cabab
>--- /dev/null
>+++ b/JSTests/stress/tail-call-with-spilled-registers.js
>@@ -0,0 +1,51 @@
>+//@ run("--useConcurrentJIT=false")
>+
>+"use strict";
>+
>+function assert(a, e) {
>+  if (a !== e)
>+    throw new Error('Expected: ' + e + ' but got: ' + a);
>+}
>+noInline(assert);
>+
>+function c3(v, b, c, d, e) {
>+    return v + b + c + d + e;
>+}
>+noInline(c3);
>+
>+function c1(o) {
>+    let ret = o.c2;
>+    if (o.a)
>+      assert(o.a, 126);
>+    return o;
>+}
>+noInline(c1);
>+
>+function getter() {
>+    let b = Math.random();
>+    let c = Math.random();
>+    let d = Math.random();
>+    let e = Math.random();
>+    return c3('test', b, c, d, e);
>+}
>+noInline(getter);
>+
>+let c = [];
>+
>+c[0] = {a: 126};
>+c[0].foo = 0;
>+c[0].c2 = 15;
>+
>+c[1] = {};
>+c[1].bar = 99;
>+
>+c[2] = {};
>+Object.defineProperty(c[2], 'c2', { get: getter });
>+
>+for (let i = 0; i < 10000; i++) {
>+    if (numberOfDFGCompiles(c1) > 0)
>+        c1(c[2]);
>+    else
>+        c1(c[i % 2]);
>+}
>+
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index eeb5b5c5aba..536113976f0 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-05-14  Caio Lima  <ticaiolima@gmail.com>
>+
>+        [ARMv7] Tail calls are broken on THUMB2
>+        https://bugs.webkit.org/show_bug.cgi?id=197797
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch is fixing the problem when we calculate stack
>+        re-usage on tail calls on ARMv7. This operation considers that frame
>+        register (cfr) is stack-aligned (i.e 16-bytes aligned). This is not
>+        true on ARMv7 case, because function's prologue pushes `r7` and `lr`
>+        to the stack, and since those registers are 4-bytes, it will result in a
>+        frame pointer that is 8-bytes off. Such disaligment may corrupt
>+        stack, because calle's frame can clobber part of the frame just before the
>+        replaced one.
>+        This path is chaging "prepare for tail call" operation on ARMv7 to adjust
>+        frame pointer to be aligned (i.e bumping it by 8 bytes) and avoid that
>+        calee's frame of a tail call corrupt the stack.
>+
>+        * jit/CallFrameShuffler.cpp:
>+        (JSC::CallFrameShuffler::prepareForTailCall):
>+        * llint/LowLevelInterpreter.asm:
>+
> 2019-05-12  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Compress Watchpoint size by using enum type and Packed<> data structure
>diff --git a/Source/JavaScriptCore/jit/CCallHelpers.h b/Source/JavaScriptCore/jit/CCallHelpers.h
>index e21286f6bbc..219517e8b11 100644
>--- a/Source/JavaScriptCore/jit/CCallHelpers.h
>+++ b/Source/JavaScriptCore/jit/CCallHelpers.h
>@@ -782,6 +782,14 @@ public:
>                 mul32(TrustedImm32(sizeof(Register)), oldFrameSizeGPR, oldFrameSizeGPR);
>             }
> 
>+#if CPU(ARM_THUMB2)
>+            // This adjust is necessary because frame pointer is not
>+            // stack-aligned on ARM 32-bits. Since its registers are 4 bytes
>+            // after function's prologue the framePointer register will be 8-bytes
>+            // off.
>+            subPtr(TrustedImm32(sizeof(Register)), oldFrameSizeGPR);
>+#endif
>+
>             // The new frame pointer is at framePointer + oldFrameSize - newFrameSize
>             ASSERT(newFramePointer != oldFrameSizeGPR);
>             addPtr(framePointerRegister, oldFrameSizeGPR, newFramePointer);
>diff --git a/Source/JavaScriptCore/jit/CallFrameShuffler.cpp b/Source/JavaScriptCore/jit/CallFrameShuffler.cpp
>index 02e372cdfef..97990994dfa 100644
>--- a/Source/JavaScriptCore/jit/CallFrameShuffler.cpp
>+++ b/Source/JavaScriptCore/jit/CallFrameShuffler.cpp
>@@ -444,6 +444,13 @@ void CallFrameShuffler::prepareForTailCall()
>         m_newFrameBase);
>     done.link(&m_jit);
> 
>+#if CPU(ARM_THUMB2)
>+    // This is needed because `cfr` is not stack-aligned on this architecture.
>+    // It is off by 8-bytes and we need to take this in consideration before
>+    // reusing the frame.
>+    m_jit.subPtr(MacroAssembler::TrustedImm32(sizeof(Register)), m_newFrameBase);
>+#endif
>+
>     m_jit.addPtr(GPRInfo::callFrameRegister, m_newFrameBase);
>     m_jit.subPtr(
>         MacroAssembler::TrustedImm32(
>diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
>index bc419ed6179..6ae0416e6cb 100644
>--- a/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
>+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
>@@ -928,6 +928,13 @@ macro prepareForTailCall(callee, temp1, temp2, temp3, callPtrTag)
>     addi StackAlignment - 1 + CallFrameHeaderSize, temp2
>     andi ~StackAlignmentMask, temp2
> 
>+    if ARMv7
>+        # This adjustment is necessary because `cfr` is not stack-aligned.
>+        # This happens because function's prologue pushes `lr` and `cfr`
>+        # to the stack, resulting in a `cfr` that is off by 8-bytes.
>+        subi CallerFrameAndPCSize, temp2
>+    end
>+
>     move cfr, temp1
>     addp temp2, temp1
>

Flags:

guijemont+jsc-armv7-ews: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 197797: 369606 | 369650 | 369895 | 369981 | 370019 | 370021 | 370650 | 370651 | 370661 | 370784 | 370785 | 371567 | 371580 | 392300 | 393034