Attachment 370784 Details for Bug 197797 – WIP - Patch

[patch] WIP - Patch

bug-197797-20190528233631.patch (text/plain), 6.11 KB, created by Caio Lima on 2019-05-28 14:36:33 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Caio Lima

Created: 2019-05-28 14:36:33 PDT

Size: 6.11 KB

patch

obsolete

>Subversion Revision: 245807
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 6513eb292c980a15e8babd5091005f4b61ec8718..d2b8695e9e4cec5ef033727221027ad9d9bfa81a 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,84 @@
>+2019-05-28  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 was 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 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 changing the alignment of ARMv7 and MIPS to 8 bytes to
>+        reflect their current alignment and avoid stack corruption.
>+
>+        * runtime/StackAlignment.h:
>+        (JSC::stackAlignmentBytes):
>+
> 2019-05-27  Tadeu Zagallo  <tzagallo@apple.com>
> 
>         Fix opensource build of testapi
>diff --git a/Source/JavaScriptCore/runtime/StackAlignment.h b/Source/JavaScriptCore/runtime/StackAlignment.h
>index 51025c0173537f94f1a13241af79f72b402b0150..85af19baedd267f376ce6de3d0b18b67b179812b 100644
>--- a/Source/JavaScriptCore/runtime/StackAlignment.h
>+++ b/Source/JavaScriptCore/runtime/StackAlignment.h
>@@ -32,7 +32,14 @@
> namespace JSC {
> 
> // NB. Different platforms may have different requirements here. But 16 bytes is very common.
>-constexpr unsigned stackAlignmentBytes() { return 16; }
>+constexpr unsigned stackAlignmentBytes()
>+{
>+#if CPU(ARM_THUMB2) || CPU(MIPS)
>+    return 8;
>+#else
>+    return 16;
>+#endif
>+}
> 
> constexpr unsigned stackAlignmentRegisters()
> {
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 96d083c93a85dfc81c589526c3a439dbd8bd881a..42832ce1ca6fcbce2bed47ba2905d24c637c33d3 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2019-05-28  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-05-25  Tadeu Zagallo  <tzagallo@apple.com>
> 
>         JITOperations getByVal should mark negative array indices as out-of-bounds
>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