Bug 181739 – Update the argument count in DFGByteCodeParser::handleRecursiveCall

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

[Robin Morisset]

handleRecursiveCall can optimize tail calls with less arguments than the original caller, if we are not in an inlined call frame.
In such a situation, we need to update the argument count, so that accesses to 'arguments.length' return the right value.

It is a bit tricky, as the argument count is in a 32-bit slot on the stack, sharing its 64-bit slot with the code origin.
So I am considering either adding a new SetArgumentCountIncludingThis node, and supporting it in both DFG and FTL; or supporting PutStack in DFG (it is currently only supported in FTL) and using MovHint+PutStack, since PutStack seems to be able to store a single int32.

<rdar://problem/36120501>

[Robin Morisset]

After a second look, it appears that SetLocal can also do a 32-bit store, provided its input is marked as FlushedInt32. I will test it to see if I can avoid messing with either a new node or PutStack.

[Robin Morisset]

Phil remarked that while SetLocal and PutStack would have worked as they are currently implemented, semantically they only support 64-bit stores and so it would not be sound to use them for this purpose. So I will add a new SetArgumentCountIncludingThis node as originally suggested by Saam.

[Robin Morisset]

Created attachment 331530 [details]
Patch

[Saam Barati]

Comment on attachment 331530 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1400
> +            }

This is wrong. Please add a test. The machine argument count is not known at compile time. We must always update it.

[Robin Morisset]

Created attachment 331609 [details]
Patch

[Robin Morisset]

(In reply to Saam Barati from comment #4)
> Comment on attachment 331530 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331530&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1400
> > +            }
> 
> This is wrong. Please add a test. The machine argument count is not known at
> compile time. We must always update it.

Thanks for the catch, I had misunderstood the role of m_codeBlock->numParameters().

I've fixed it, and added a second test for this.

[EWS Watchlist]

Comment on attachment 331609 [details]
Patch

Attachment 331609 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6120464

New failing tests:
stress/inline-call-to-recursive-tail-call.js.ftl-no-cjit-no-inline-validate
stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit
stress/tail-call-no-stack-overflow.js.no-cjit-validate-phases
stress/inline-call-to-recursive-tail-call.js.ftl-no-cjit-no-put-stack-validate
stress/inline-call-to-recursive-tail-call.js.dfg-maximal-flush-validate-no-cjit
stress/v8-earley-boyer-strict.js.dfg-eager-no-cjit-validate
stress/dfg-tail-calls.js.dfg-maximal-flush-validate-no-cjit
stress/async-iteration-basic.js.dfg-eager-no-cjit-validate
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager-no-cjit
stress/dfg-tail-calls.js.no-cjit-validate-phases
stress/dfg-tail-calls.js.ftl-no-cjit-no-inline-validate
stress/inline-call-to-recursive-tail-call.js.ftl-eager-no-cjit
stress/inline-call-to-recursive-tail-call.js.ftl-no-cjit-validate-sampling-profiler
stress/dfg-tail-calls.js.dfg-eager-no-cjit-validate
stress/v8-earley-boyer-strict.js.no-cjit-validate-phases
stress/async-iteration-basic.js.no-cjit-validate-phases
stress/tail-call-no-stack-overflow.js.ftl-no-cjit-no-put-stack-validate
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager
stress/inline-call-to-recursive-tail-call.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-async-from-sync.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-yield-star.js.ftl-no-cjit-no-put-stack-validate
stress/inline-call-to-recursive-tail-call.js.dfg-eager-no-cjit-validate
stress/tail-call-no-stack-overflow.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-basic.js.ftl-eager-no-cjit
stress/v8-earley-boyer-strict.js.dfg-maximal-flush-validate-no-cjit
stress/tail-call-no-stack-overflow.js.ftl-no-cjit-validate-sampling-profiler
stress/async-iteration-yield-promise.js.ftl-eager-no-cjit-b3o1
stress/tail-call-no-stack-overflow.js.dfg-eager-no-cjit-validate
stress/async-iteration-basic.js.dfg-maximal-flush-validate-no-cjit
stress/v8-earley-boyer-strict.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-yield-star.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-yield-star-interface.js.ftl-eager-no-cjit-b3o1
stress/dfg-tail-calls.js.ftl-no-cjit-no-put-stack-validate
stress/async-iteration-yield-star.js.no-cjit-validate-phases
stress/v8-earley-boyer-strict.js.ftl-no-cjit-no-put-stack-validate
stress/recursive-tail-call-with-different-argument-count.js.dfg-maximal-flush-validate-no-cjit
stress/dfg-tail-calls.js.ftl-no-cjit-validate-sampling-profiler
stress/async-iteration-async-from-sync.js.dfg-eager-no-cjit-validate
stress/tail-call-no-stack-overflow.js.ftl-no-cjit-no-inline-validate
stress/dfg-tail-calls.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-yield-star.js.dfg-eager-no-cjit-validate
stress/async-iteration-async-from-sync.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.no-cjit-validate-phases
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-no-put-stack-validate
stress/dfg-tail-calls.js.ftl-eager-no-cjit
stress/async-iteration-basic.js.ftl-no-cjit-no-put-stack-validate
stress/inline-call-to-recursive-tail-call.js.ftl-eager
stress/v8-earley-boyer-strict.js.ftl-no-cjit-validate-sampling-profiler
stress/tail-call-no-stack-overflow.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.dfg-eager-no-cjit-validate
stress/async-iteration-basic.js.ftl-no-cjit-validate-sampling-profiler
stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-validate-sampling-profiler
stress/inline-call-to-recursive-tail-call.js.no-cjit-validate-phases
stress/async-iteration-yield-star.js.ftl-no-cjit-validate-sampling-profiler
stress/tail-call-no-stack-overflow.js.dfg-maximal-flush-validate-no-cjit
stress/v8-earley-boyer-strict.js.default
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-b3o1
stress/async-iteration-yield-star.js.dfg-maximal-flush-validate-no-cjit
stress/async-iteration-yield-star.js.ftl-eager
stress/async-iteration-basic.js.ftl-eager-no-cjit-b3o1

[Robin Morisset]

Created attachment 331641 [details]
Patch

[Saam Barati]

Comment on attachment 331641 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        It required adding a new DFG node: 'SetArgumentCountIncludingThis', that takes an unsigned int
> +        as its first OpInfo field, and stores it to the stack at the right place.

Please also add a description of the bug here. This just states what your solution is. It doesn't go into enough detail about what was wrong with the initial implementation.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1405
> +        // We must update the argument count on the stack with 'SetArgumentCountIncludingThis' instead of SetLocal,
> +        // because it is a 32-bit slot on the stack, and not a normal (64 bit) js value.

This comment does not add any real info IMO. I think it falls out just from reading the code.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11303
> +    m_jit.store32(TrustedImm32(node->argumentCountIncludingThis()), JITCompiler::payloadFor(CallFrameSlot::argumentCount));

You need to add
noResult(node)
here.

[Saam Barati]

<rdar://problem/36627662>

[EWS Watchlist]

Comment on attachment 331641 [details]
Patch

Attachment 331641 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6123503

New failing tests:
stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager-no-cjit
stress/inline-call-to-recursive-tail-call.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-eager
stress/inline-call-to-recursive-tail-call.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-basic.js.ftl-eager-no-cjit
stress/v8-earley-boyer-strict.js.ftl-no-cjit-validate-sampling-profiler
stress/async-iteration-yield-star.js.ftl-eager-no-cjit
stress/v8-earley-boyer-strict.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-yield-star.js.ftl-eager-no-cjit-b3o1
stress/v8-earley-boyer-strict.js.default
stress/async-iteration-async-from-sync.js.ftl-eager-no-cjit
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-no-put-stack-validate
stress/async-iteration-yield-promise.js.ftl-eager-no-cjit-b3o1
stress/v8-earley-boyer-strict.js.ftl-eager
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-validate-sampling-profiler
stress/inline-call-to-recursive-tail-call.js.ftl-eager
stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit-b3o1
stress/async-iteration-yield-star-interface.js.ftl-eager-no-cjit-b3o1
stress/recursive-tail-call-with-different-argument-count.js.ftl-no-cjit-b3o1
stress/inline-call-to-recursive-tail-call.js.ftl-no-cjit-no-inline-validate
stress/async-iteration-yield-star.js.ftl-eager
stress/async-iteration-basic.js.ftl-eager-no-cjit-b3o1

[Robin Morisset]

Created attachment 331947 [details]
Patch

[Saam Barati]

Comment on attachment 331947 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 331947 [details]
Patch

Clearing flags on attachment: 331947

Committed r227410: <https://trac.webkit.org/changeset/227410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.