[webkit-reviews] review granted: [Bug 181739] Update the argument count in DFGByteCodeParser::handleRecursiveCall : [Attachment 331641] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 181739: Update the argument count in DFGByteCodeParser::handleRecursiveCall
https://bugs.webkit.org/show_bug.cgi?id=181739

Attachment 331641: Patch

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




--- Comment #9 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 331641
  --> https://bugs.webkit.org/attachment.cgi?id=331641
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.