Bug 179835 – The recursive tail call optimisation is wrong on closures

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

Created attachment 327215 [details]
testcase: merge-sort-cps.js

We speculate on the executable of the callee, instead of the callee itself. And two different instances of a closure share the same executable even when they have different captured variables.
This bug manifests as a non-deterministic infinite loop in merge-sort-cps.js (attached).
This happens when the only value that has been seen for cont in "cont(result)" is the "(right) => {..}" closure, because it is then turned into an infinite loop.

The problem is in the use of emitFunctionChecks: that function only checks the executable of the callee. If we checked for the callee itself in the case of closures, there would be no such problems.

[Robin Morisset]

Created attachment 327645 [details]
Patch

[Saam Barati]

Comment on attachment 327645 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1436
> +    // Currently we cannot do this optimisation for closures. The problem is that "emitFunctionChecks" which we use later is too coarse, only checking the executable
> +    // and not the value of captured variables.
> +    if (callLinkStatus[0].isClosureCall())
> +        return false;

How is this only a problem with this code an not all of inlining? Can you elaborate? Is the bug if we constant fold loading a closure variable?

Can you add a semantic test for this and not just a benchmark?

[Robin Morisset]

Created attachment 327852 [details]
Patch

[Robin Morisset]

I have rebased the patch so it applies cleanly, and added a small stress test as requested.

How inlining avoids this problem is interesting and quite subtle:
- The variable is taken from a GetScope which in turn comes from a GetCallee.
- From DFGSpeculativeJIT, we see that GetCallee takes its value from origin.inlineCallFrame->calleeRecovery.virtualRegister() in the case where origin.inlineCallFrame->isClosureCall.
- inlineCallFrame->isClosureCall is set in the constructor of inlineStackEntry, called by inlineCall()
- inlineCallFrame->calleeRecovery is set in DFGStackLayoutPhase, from InlineVariableData::calleeVariable
- Finally, InlineVariableData::calleeVariable is set in inlineCall() in the DFGByteCodeParser.cpp

It is not clear at all how any of this machinery may be reused for this optimization.
So my current plan is to disable the optimization for closures altogether as a stopgap (this patch), and then try in https://bugs.webkit.org/show_bug.cgi?id=180044 to add it back a lot more carefully, probably with some explicit branch on the callee.

[WebKit Commit Bot]

Comment on attachment 327852 [details]
Patch

Clearing flags on attachment: 327852

Committed r225270: <https://trac.webkit.org/changeset/225270>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35749564>