Bug 197405 – [JSC] Inlining Getter/Setter should care availability of ad-hocly constructed frame

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Currenlty, we are setting up arguments for getter/setter callings before doing handleCall. But this is wrong. Let’s consider the following case,

  var counter = 0;
  var o = {
    get f() {
      return o
    },
    set f(v) {
      counter++;
      this.z = 0;
    }
  };
  function foo(o) {
    o.f = 0; // setter call. Inlining in DFG.
    return o.f; // getter call. Inlining in DFG.
  }
  noInline(foo);
  foo(o);

BB#0
    ...
    19:<!0:-> MovHint(Check:Untyped:@18, MustGen, loc4, W:SideState, ClobbersExit, bc#1, ExitValid)
    20:< 1:-> SetLocal(Check:Untyped:@18, loc4(J~/FlushedJSValue), W:Stack(-5), bc#1, exit: bc#3, ExitValid)  predicting None
    21:<!0:-> MovHint(Check:Untyped:@18, MustGen, loc5, W:SideState, ClobbersExit, bc#3, ExitValid)
    22:< 1:-> SetLocal(Check:Untyped:@18, loc5(K~/FlushedJSValue), W:Stack(-6), bc#3, exit: bc#6, ExitValid)  predicting None
    ...
    30:< 1:-> GetSetter(Check:Untyped:@29, JS|PureInt, R:GetterSetter_setter, Exits, bc#7, ExitValid)
    ...
    37:< 1:-> GetExecutable(Check:Untyped:@30, JS|PureInt, Exits, bc#7, ExitValid)
    ...
    /* Multiple CallVariants. So let's switch on executables. */
    ...
    41:<!0:-> Switch(Check:Untyped:@37, MustGen, SwitchCell, Weak:Cell: 0x1057c3680 (%Ei:FunctionExecutable), StructureID: 18859:#2, default:#3, W:SideState, Exits, bc#7, ExitValid)

BB#2
    42:<!0:-> GetLocal(JS|MustGen|PureInt, loc12(O~/FlushedJSValue), R:Stack(-13), bc#7, ExitValid)  predicting None
    43:<!0:-> MovHint(Check:Untyped:@42, MustGen, loc12, W:SideState, ClobbersExit, bc#7, ExitValid)
    44:< 1:-> SetLocal(Check:Untyped:@42, loc12(P!/FlushedJSValue), W:Stack(-13), bc#7, ExitInvalid)  predicting None
    --> f#DMIaZz:<0x1057a0390, bc#7, SetterCall, closure call, numArgs+this = 2, numFixup = 0, stackOffset = -16 (loc0 maps to loc16)>
      45:<!0:->   ExitOK(MustGen, R:Stack(-13), W:SideState, bc#0, ExitValid)
      ...
      <HERE>


If OSR exit occurs <HERE>, we construct Stack based on availability. But now, loc4 & loc5 (|this| and |arg1| for setter) availability can be pruned at the beginning of BB#2 since bc#7 do not need to make them live!

[Yusuke Suzuki]

<rdar://problem/50008714>

[Yusuke Suzuki]

Created attachment 368542 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 368594 [details]
Patch

[Yusuke Suzuki]

Created attachment 368596 [details]
Patch

[Yusuke Suzuki]

Created attachment 368622 [details]
Patch

[Yusuke Suzuki]

Created attachment 368627 [details]
Patch

[Saam Barati]

Comment on attachment 368627 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
> +        // BB#2's first node is @42, which origin is bc#7 of the caller (not callee f#DMIaZz). So OSR availability phase can prune @19 and @21 liveness, because these stack values are set up

there is no @21.

[Saam Barati]

Comment on attachment 368627 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1614
> +        // When inlining getter and setter calls, we setup the stack frame which does not appear in the bytecode.

"the stack frame" => "a stack frame"

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1615
> +        // But inlining call can have switch on executable. So we could have a graph like this.

"But inlining call can have switch on executable. So we could have a graph like this." => "Because Inlining can switch on executable, we could have a graph like this."

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1634
> +        //     19:<!0:-> MovHint(Check:Untyped:@18, MustGen, loc4, W:SideState, ClobbersExit, bc#1, ExitValid)
> +        //     20:< 1:-> SetLocal(Check:Untyped:@18, loc4(J~/FlushedJSValue), W:Stack(-5), bc#1, exit: bc#3, ExitValid)  predicting None
> +        //     ...
> +        //     30:< 1:-> GetSetter(Check:Untyped:@29, JS|PureInt, R:GetterSetter_setter, Exits, bc#7, ExitValid)
> +        //     ...
> +        //     37:< 1:-> GetExecutable(Check:Untyped:@30, JS|PureInt, Exits, bc#7, ExitValid)
> +        //     ...
> +        //     41:<!0:-> Switch(Check:Untyped:@37, MustGen, SwitchCell, Weak:Cell: 0x1057c3680 (%Ei:FunctionExecutable), StructureID: 18859:#2, default:#3, W:SideState, Exits, bc#7, ExitValid)
> +        //
> +        // BB#2
> +        //     42:<!0:-> GetLocal(JS|MustGen|PureInt, loc12(O~/FlushedJSValue), R:Stack(-13), bc#7, ExitValid)  predicting None
> +        //     43:<!0:-> MovHint(Check:Untyped:@42, MustGen, loc12, W:SideState, ClobbersExit, bc#7, ExitValid)
> +        //     44:< 1:-> SetLocal(Check:Untyped:@42, loc12(P!/FlushedJSValue), W:Stack(-13), bc#7, ExitInvalid)  predicting None
> +        //     --> f#DMIaZz:<0x1057a0390, bc#7, SetterCall, closure call, numArgs+this = 2, numFixup = 0, stackOffset = -16 (loc0 maps to loc16)>
> +        //       45:<!0:->   ExitOK(MustGen, R:Stack(-13), W:SideState, bc#0, ExitValid)

This has more detail than needed IMO. However, because you're changing this, it'd important to commend on how this gets changed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
>> +        // BB#2's first node is @42, which origin is bc#7 of the caller (not callee f#DMIaZz). So OSR availability phase can prune @19 and @21 liveness, because these stack values are set up
> 
> there is no @21.

I think the key just boils down to:
we need MovHints in the callee's code origin at the start of our code because those can't be pruned.
The caller's code, because these are manufactured locals, can always prune them (which may have some other bugs the more I think about it)

[Saam Barati]

Comment on attachment 368627 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
>>> +        // BB#2's first node is @42, which origin is bc#7 of the caller (not callee f#DMIaZz). So OSR availability phase can prune @19 and @21 liveness, because these stack values are set up
>> 
>> there is no @21.
> 
> I think the key just boils down to:
> we need MovHints in the callee's code origin at the start of our code because those can't be pruned.
> The caller's code, because these are manufactured locals, can always prune them (which may have some other bugs the more I think about it)

The wording in this paragraph should address what this fixes. It shouldn't say that we still do the wrong thing in the present tense. Maybe something like:


```
If we prune OSR availability at bc#7 in the caller, we can prune @19 and @21. When we begin executing the callee, we
need OSR exit to be aware of where it can recover the arguments to the setter, loc4 and loc5. The MovHints in the inlined
callee make it so that if we exit at <HERE>, we can recover loc4/loc5.
```

[Saam Barati]

Comment on attachment 368627 [details]
Patch

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

>>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
>>>> +        // BB#2's first node is @42, which origin is bc#7 of the caller (not callee f#DMIaZz). So OSR availability phase can prune @19 and @21 liveness, because these stack values are set up
>>> 
>>> there is no @21.
>> 
>> I think the key just boils down to:
>> we need MovHints in the callee's code origin at the start of our code because those can't be pruned.
>> The caller's code, because these are manufactured locals, can always prune them (which may have some other bugs the more I think about it)
> 
> The wording in this paragraph should address what this fixes. It shouldn't say that we still do the wrong thing in the present tense. Maybe something like:
> 
> 
> ```
> If we prune OSR availability at bc#7 in the caller, we can prune @19 and @21. When we begin executing the callee, we
> need OSR exit to be aware of where it can recover the arguments to the setter, loc4 and loc5. The MovHints in the inlined
> callee make it so that if we exit at <HERE>, we can recover loc4/loc5.
> ```

maybe also continue to say in the first sentence why they can be pruned.

[Yusuke Suzuki]

Comment on attachment 368627 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1614
>> +        // When inlining getter and setter calls, we setup the stack frame which does not appear in the bytecode.
> 
> "the stack frame" => "a stack frame"

Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1615
>> +        // But inlining call can have switch on executable. So we could have a graph like this.
> 
> "But inlining call can have switch on executable. So we could have a graph like this." => "Because Inlining can switch on executable, we could have a graph like this."

Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1634
>> +        //       45:<!0:->   ExitOK(MustGen, R:Stack(-13), W:SideState, bc#0, ExitValid)
> 
> This has more detail than needed IMO. However, because you're changing this, it'd important to commend on how this gets changed.

OK, simplified.

>>>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
>>>>> +        // BB#2's first node is @42, which origin is bc#7 of the caller (not callee f#DMIaZz). So OSR availability phase can prune @19 and @21 liveness, because these stack values are set up
>>>> 
>>>> there is no @21.
>>> 
>>> I think the key just boils down to:
>>> we need MovHints in the callee's code origin at the start of our code because those can't be pruned.
>>> The caller's code, because these are manufactured locals, can always prune them (which may have some other bugs the more I think about it)
>> 
>> The wording in this paragraph should address what this fixes. It shouldn't say that we still do the wrong thing in the present tense. Maybe something like:
>> 
>> 
>> ```
>> If we prune OSR availability at bc#7 in the caller, we can prune @19 and @21. When we begin executing the callee, we
>> need OSR exit to be aware of where it can recover the arguments to the setter, loc4 and loc5. The MovHints in the inlined
>> callee make it so that if we exit at <HERE>, we can recover loc4/loc5.
>> ```
> 
> maybe also continue to say in the first sentence why they can be pruned.

Fixed.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1653
>      if (arityFixupCount) {

Discussed with Saam on IRC, we should move the above and arity fixup into callee.

[EWS Watchlist]

Comment on attachment 368627 [details]
Patch

Attachment 368627 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12046604

New failing tests:
workers/bomb.html

[EWS Watchlist]

Created attachment 368643 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Created attachment 368644 [details]
Patch

[Saam Barati]

Comment on attachment 368644 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1694
> +                VirtualRegister argumentToSet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffsetAfterFixup));

nit: as we spoke about on IRC, we think this can be:
m_inlineStackTop->remapOperand(virtualRegisterForArgument(index))

[Saam Barati]

Comment on attachment 368644 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1648
> +            VirtualRegister argumentToGet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffset));

nit: I think same thing here w.r.t remapOperand.

[EWS Watchlist]

Comment on attachment 368644 [details]
Patch

Attachment 368644 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12047953

New failing tests:
http/tests/preload/download_resources_from_header_iframe.html

[EWS Watchlist]

Created attachment 368649 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 368644 [details]
Patch

Attachment 368644 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12047895

New failing tests:
imported/w3c/web-platform-tests/streams/readable-streams/general.dedicatedworker.html
imported/w3c/web-platform-tests/streams/readable-streams/general.html

[EWS Watchlist]

Created attachment 368650 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 368644 [details]
Patch

Attachment 368644 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12047773

New failing tests:
wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-tls-context

[Yusuke Suzuki]

(In reply to Build Bot from comment #21)
> Comment on attachment 368644 [details]
> Patch
> 
> Attachment 368644 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12047773
> 
> New failing tests:
> wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
> wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
> wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-
> tls-context

Looking this now. It seems this is real.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #22)
> (In reply to Build Bot from comment #21)
> > Comment on attachment 368644 [details]
> > Patch
> > 
> > Attachment 368644 [details] did not pass jsc-ews (mac):
> > Output: https://webkit-queues.webkit.org/results/12047773
> > 
> > New failing tests:
> > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
> > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
> > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-
> > tls-context
> 
> Looking this now. It seems this is real.

Moving arity fixup causes this issue. I'm investigating what is going on.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #23)
> (In reply to Yusuke Suzuki from comment #22)
> > (In reply to Build Bot from comment #21)
> > > Comment on attachment 368644 [details]
> > > Patch
> > > 
> > > Attachment 368644 [details] did not pass jsc-ews (mac):
> > > Output: https://webkit-queues.webkit.org/results/12047773
> > > 
> > > New failing tests:
> > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
> > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
> > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-
> > > tls-context
> > 
> > Looking this now. It seems this is real.
> 
> Moving arity fixup causes this issue. I'm investigating what is going on.

In loop_hint, something super unrelated local is live in DFG and dead in bytecode, which leads to crash in OSR entry.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #24)
> (In reply to Yusuke Suzuki from comment #23)
> > (In reply to Yusuke Suzuki from comment #22)
> > > (In reply to Build Bot from comment #21)
> > > > Comment on attachment 368644 [details]
> > > > Patch
> > > > 
> > > > Attachment 368644 [details] did not pass jsc-ews (mac):
> > > > Output: https://webkit-queues.webkit.org/results/12047773
> > > > 
> > > > New failing tests:
> > > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
> > > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
> > > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-
> > > > tls-context
> > > 
> > > Looking this now. It seems this is real.
> > 
> > Moving arity fixup causes this issue. I'm investigating what is going on.
> 
> In loop_hint, something super unrelated local is live in DFG and dead in
> bytecode, which leads to crash in OSR entry.

I believe this is bug in DFGLiveCatchVariablePreservationPhase, I'll discuss it tomorrow (precisely, today, lol).

[Yusuke Suzuki]

Comment on attachment 368644 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1697
>              }

These SetLocals are somewhat "state bootstrapping" ones. Previously, we set caller's CodeOrigin. So these SetLocals are always putting them in locals, and it is not shown as put to |this|
Now, we have code origin belonging to the caller. So |this| put / move is shown as SetLocal(arg0). And then, we insert invalid Flush before this in DFGLiveCatchVariablePreservationPhase.
I have one question. Why does DFGLiveCatchVariablePreservationPhase flushes all the |this| in the seen InlinedCallFrame regardless of the liveness at the catch?

[Saam Barati]

I think you want the NodeOrigin of your SetLocals to be what they were before:
- semantic in caller
- forExit in callee

[Saam Barati]

I guess that actually goes against the pruning issue. How you have it now makes sense actually. Let’s figure out what the issue is.

[Yusuke Suzuki]

Discussed with Saam and Phil, we should tell some interesting liveness information at op_enter for locals that are materialized by DFG (getter / setter arguments, arity fixups).

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #29)
> Discussed with Saam and Phil, we should tell some interesting liveness
> information at op_enter for locals that are materialized by DFG (getter /
> setter arguments, arity fixups).

Discussed with Saam about DFGLiveCatchVariablePreservationPhase, and I think the fundamental problem is DFGLiveCatchVariablePreservationPhase's InlineCallFrame handling. I'll upload the patch with this fix soon.

[Yusuke Suzuki]

Created attachment 368744 [details]
Patch

[Yusuke Suzuki]

Committed r244864: <https://trac.webkit.org/changeset/244864>