Attachment 371567 Details for Bug 197797 – Patch

[patch] Patch

bug-197797-20190607085732.patch (text/plain), 6.85 KB, created by Caio Lima on 2019-06-06 23:57:34 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Caio Lima

Created: 2019-06-06 23:57:34 PDT

Size: 6.85 KB

patch

obsolete

>Subversion Revision: 246187
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 4615ea2cfd989f912ada374f138af1a30ab94ce0..65719d80275a2e9f25a066cf59ed0edbe7ef37b4 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,85 @@
>+2019-06-06  Caio Lima  <ticaiolima@gmail.com>
>+
>+        Tail calls are broken on ARM_THUMB2 and MIPS
>+        https://bugs.webkit.org/show_bug.cgi?id=197797
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        JSC's stack alignment is 16 bytes for every supported architecture.
>+        However, this is not preserved by function's prologue on ARMv7 and MIPS,
>+        because frame pointer and `lr` are pushed into stack during this operation,
>+        resulting in a frame that is not aligned.
>+        This is a problem when we are calculating top of the stack on some
>+        cases of tail calls, because it considers that frames are stack-aligned.
>+        Such disaligment may corrupt stack values when a tail call happens
>+        inside a getter execution.
>+        Let's consider the situation below with the following script:
>+
>+        ```
>+        let o = {
>+            get x() {
>+                return tail_call();
>+            }
>+        }
>+
>+        let foo = (o) => {
>+            ...
>+            return o.x == o.y;
>+        }
>+        ```
>+
>+        Also, lets suppose that we compiled `foo` into DFG and we generated IC
>+        code for `o.x` and `o.y`. The register `r1` is assigned to `o`.
>+        Since the IC code of `o.x` is a getter case, this code will spill `r1`
>+        to the stack to proper setup the JS call and restore
>+        its value when the getter returns from execution (see third line of stack below).
>+        The getter IC code allocates 32 bytes to call the JS getter, and during the
>+        getter's prologue, `lr` and `cfr` are pushed to the stack, ending up
>+        with a new frame pointed by `cfr1`. During tail call preparation, we
>+        then calculate the top of stack using the following operations:
>+
>+        ```
>+        muli SlotSize, argc # Slot size is 8
>+        # StackAlignment = 16
>+        # CallFrameHeaderSize = 32
>+        addi StackAlignment - 1 + CallFrameHeaderSize, argcInBytes
>+        andi ~StackAlignmentMask, argcInBytes
>+
>+        move cfr, newCFR
>+        addp argcInBytes, newCFR
>+        ```
>+
>+        Considering `argc = 1` in the operation above, the result is then `argcInBytes = 48`.
>+        Adding 48 bytes to `newCFR` will make the top of the stack off by 8 bytes,
>+        since current stack frame is 40 bytes (32 from caller allocation + 4 of `lr`
>+        + 4 of `cfr`).
>+        In the end, when we start the copy
>+        operation of the stack, the address where `r1` is spilled will be
>+        cloberred, generating unexpected results.
>+
>+        +----------------+ <-- Stack of program execution. Each cell is 4 bytes.
>+        |      ...       |
>+        +----------------+ <-- newCFR (where the calculation of top of stack will result)
>+        |      ...       |
>+        +----------------+
>+        |      r1        |
>+        +----------------+ <-- Start of stack allocated to call getter
>+        |      ...       | |
>+        +----------------+ | 32 bytes
>+        |      ...       | |
>+        +----------------+ <-- End of stack allocated to call getter
>+        |      lr        |
>+        +----------------+
>+        |      cfr       |
>+        +----------------+ <-- cfr1
>+
>+        This patch is adding 8 bytes on MIPS and ARMv7 when allocatting stack space for
>+        generated IC code for accessor cases. This way, we guarantee that tail
>+        call's stack reusage won't clobber values on stack.
>+
>+        * bytecode/AccessCase.cpp:
>+        (JSC::AccessCase::generateImpl):
>+
> 2019-06-06  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: create CommandLineAPIHost lazily like the other agents
>diff --git a/Source/JavaScriptCore/bytecode/AccessCase.cpp b/Source/JavaScriptCore/bytecode/AccessCase.cpp
>index c10859aced3db78b664976a08b8e5b48d652b2f3..af5467bdd8076e25244348a53d5cc05d38bac69e 100644
>--- a/Source/JavaScriptCore/bytecode/AccessCase.cpp
>+++ b/Source/JavaScriptCore/bytecode/AccessCase.cpp
>@@ -891,7 +891,15 @@ void AccessCase::generateImpl(AccessGenerationState& state)
>                 CCallHelpers::Zero, loadedValueGPR);
> 
>             unsigned numberOfRegsForCall = CallFrame::headerSizeInRegisters + numberOfParameters;
>+#if CPU(ARM_THUMB2) || CPU(MIPS)
>+            // For ARMv7 and MIPS architectures, we need 8 extra bytes to
>+            // guarantee that stack size have enough space to be reused by a
>+            // tail call. Since sizeof(CallerFrameAndPC) == 8 for those architectures,
>+            // we only need to calculate `numberOfBytesForCall = numberOfRegsForCall * sizeof(Register)`.
>+            unsigned numberOfBytesForCall = numberOfRegsForCall * sizeof(Register);
>+#else
>             unsigned numberOfBytesForCall = numberOfRegsForCall * sizeof(Register) - sizeof(CallerFrameAndPC);
>+#endif
> 
>             unsigned alignedNumberOfBytesForCall =
>             WTF::roundUpToMultipleOf(stackAlignmentBytes(), numberOfBytesForCall);
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 59fc2acae67b97158f9c225f6b888e13a953f0fe..f19523709ecdd363bad0fecb0d636af6726c4bc1 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2019-06-06  Caio Lima  <ticaiolima@gmail.com>
>+
>+        Tail calls are broken on ARM_THUMB2 and MIPS
>+        https://bugs.webkit.org/show_bug.cgi?id=197797
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/tail-call-with-spilled-registers.js: Added.
>+
> 2019-06-05  Justin Michaud  <justin_michaud@apple.com>
> 
>         [WASM-References] Add support for Anyref tables, Table.get and Table.set (for Anyref only).
>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 0000000000000000000000000000000000000000..03fb71cababdea027249e1cf7d4859f2a99f0327
>--- /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]);
>+}
>+

Actions: View | Formatted Diff | Diff

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