[webkit-reviews] review denied: [Bug 128850] GetMyArgumentByVal in FTL : [Attachment 224444] Added GetMyArgumentByVal to FTL

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Matthew Mirman <mmirman at apple.com>'s
request for review:
Bug 128850: GetMyArgumentByVal in FTL
https://bugs.webkit.org/show_bug.cgi?id=128850

Attachment 224444: Added GetMyArgumentByVal to FTL
https://bugs.webkit.org/attachment.cgi?id=224444&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=224444&action=review


> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2047
> +	   TypedPointer argsPtr =
addressFor(m_node->origin.semantic.stackOffset());

This is the address of the first bytecode local for the current inline call
frame.	The first bytecode local has nothing to do with arguments.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2048
> +	   LValue args = argsPtr.value();

It's bad form to take the LValue directly out of the TypedPointer; it suggests
that something is broken.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2052
> +	   speculate(Uncountable, noValue(), 0, 
> +	       m_out.aboveOrEqual(m_out.add(property, m_out.constInt32(1)), 
> +		  
m_out.load32NonNegative(addressFor(m_node->origin.semantic.stackOffset() +
JSStack::ArgumentCount))));

This is wrong for inlined code.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2056
> +	   speculate(Uncountable, noValue(), 0, 
> +	       m_out.isNull(m_out.loadPtr(addressFor(args,
m_node->origin.semantic.stackOffset(),
Arguments::offsetOfSlowArgumentData()))));
> +

This is wrong.	You're loading from an address that you've computed by taking
the frame pointer, adding the stackOffset (on line 2047), then adding the
stackOffset again (like 2055), and then adding the offset of the
m_slowArguments field inside of Arguments to that.  This is a bogus pointer
value that points to some random thing on the stack.  It's meaningless to load
from this bogus location.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2060
> +	   setJSValue(m_out.load64(m_out.baseIndex(
> +	       argsPtr.heap(),
> +	       args, m_out.signExt(property, m_out.intPtr), ScaleEight, 
> +	       JSStack::ThisArgument * sizeof(Register) + sizeof(Register))));

This would have been an appropriate place to use the addressFor() method.