Bug 164351 - JSC is crashing on release mode when running exit-from-setter.js when compiled with MSVC
Summary: JSC is crashing on release mode when running exit-from-setter.js when compile...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-02 16:57 PDT by Christopher Reid
Modified: 2016-11-30 13:19 PST (History)
7 users (show)

See Also:


Attachments
WIP patch (1.88 KB, patch)
2016-11-02 18:34 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch (2.29 KB, patch)
2016-11-30 13:15 PST, Christopher Reid
fpizlo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Reid 2016-11-02 16:57:56 PDT
The RSI register is getting corrupted when exiting from the jsc vm. In MSVC this register is a nonvolatile register and used to store the scope address.

Here's what's going on:

Generated JIT code for Access stub for foo#ECI8tQ:[00000248F1BCC310->00000248F1B878A0, BaselineFunctionCall, 52 (NeverInline)] bc#24 with return point CodePtr(00000248B17F1859): Setter:(Generated, structure = 00000248F1B9C460:[Object, {_f:0, f:1}, NonArray, Proto:00000248F1B4C0A0, Leaf], offset = 1, callLinkInfo = 00000248F1B6F9C0):
    Code at [00000248B17F1EA0, 00000248B17F1F40):
		  ...
          0xb17f1eb0: sub $0x10, %rsp
          0xb17f1eb4: mov %rsi, (%rsp)
          0xb17f1eb8: mov %rdi, 0x8(%rsp)
          0xb17f1ebd: mov $0x18, 0x24(%rbp)
          0xb17f1ec4: mov 0x18(%r8), %r8
          0xb17f1ec8: test %r8, %r8
          0xb17f1ecb: jz 0x248b17f1f20
          0xb17f1ed1: sub $0x30, %rsp
          ...
>         0xb17f1f20: lea -0xa0(%rbp), %rsp
          0xb17f1f27: mov (%rsp), %rsi
          0xb17f1f2b: mov 0x8(%rsp), %rdi
          0xb17f1f30: add $0x10, %rsp
          ...

The generated access stub always assumes that RSP is set up to be a constant offset from RBP. This code works fine when run by baseline JIT but not when called by DFG JIT.

Generated Baseline JIT code for foo#ECI8tQ:[00000248F1BCC310->00000248F1B878A0, BaselineFunctionCall, 52 (NeverInline)], instruction count = 52
   Source: function foo(o_, v_) { var o = o_.f; var v = v_.f; o.f = v; o.f = v + 1; }
   Code at [00000248B17F15E0, 00000248B17F1E8B):
              0xb17f15e0: nop
              0xb17f15e1: push %rbp
              0xb17f15e2: mov %rsp, %rbp
              ...
>             0xb17f161d: lea -0x90(%rbp), %rdx
              ...
              0xb17f1637: mov %rdx, %rsp
              ...

Generated DFG JIT code for foo#ECI8tQ:[00000248F1BCCCD0->00000248F1BCC310->00000248F1B878A0, DFGFunctionCall, 52 (NeverInline)], instruction count = 52:
    Optimized with execution counter = 2550.045166/2547.000000, 3
    Code at [00000248B17F2FE0, 00000248B17F3561):
              0xb17f2fe0: push %rbp
              0xb17f2fe1: mov %rsp, %rbp
              ...
>             0xb17f300c: lea -0xb0(%rbp), %rsp
              ...

When the access stub is called by DFG JIT code, the RSP offset from RBP is different causing it to restore the wrong values in to RSI.
Comment 1 Christopher Reid 2016-11-02 17:05:03 PDT
I have tried a fix moving rsp if needed instead of using an offset of rbp, but that seems to be causing issues I'm assuming because rsp is not restored in all cases.

I don't know enough about the javascript core to know exactly what is off but I was wondering if that offset used in the DFG JIT code is correct. I was also wondering if the access stub is supposed to be regenerated with the correct offset in the DFG jit case.
Comment 2 Christopher Reid 2016-11-02 18:34:56 PDT
Created attachment 293731 [details]
WIP patch

Here's the patch I was trying to remove the frame pointer and just modifying the stack pointer but it seems like the stack pointer isn't preserved as it's causing some new tests to fail.
Comment 3 Christopher Reid 2016-11-30 13:15:20 PST
Created attachment 295747 [details]
Patch
Comment 4 Filip Pizlo 2016-11-30 13:19:21 PST
Comment on attachment 295747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295747&action=review

Please investigate why the stack pointer was not set correctly in the first place.

> Source/JavaScriptCore/ChangeLog:13
> +        What appears to have been going on was that an inline multiplication is attempted to be done in
> +        DFG jit which overflows and returns to baseline jit where the access stub is later called.
> +        When the access stub is called, the stack pointer hasn't been set up properly so that when it
> +        is restored with lea -0xXX(%rbp), %rsp, the stack pointer becomes misaligned and

An intended invariant of JSC is that the stack pointer is set correctly at every baseline JIT bytecode instruction boundary.  This implies that the bug here is that the DFG JIT is not setting the stack pointer to the appropriate height.

> Source/JavaScriptCore/ChangeLog:17
> +        There doesn't seem to be a place where the stack pointer is supposed to be set up before this
> +        call. This change sets up the stack pointer before saves based on what is expected in
> +        PolymorphicAccess.cpp:1104

There is such a place.  What you are describing sounds like a fundamental OSR exit bug, and your fix is a very weak workaround at best.  Many more things depend on the stack pointer being set right, not just PolymorphicAccess.