Arguments elimination phase mishandles arity check failure in its reduction of LoadVarargs to GetStack/PutStacks

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 253578 [details]
the patch

[Mark Lam]

<rdar://problem/21030149>

[Mark Lam]

Comment on attachment 253578 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:501
> +                        DFG_ASSERT(m_graph, node, varargsData->limit - 1 >= varargsData->mandatoryMinimum);
> +                        unsigned limit = std::max(
> +                            varargsData->limit - 1, varargsData->mandatoryMinimum);

Is the following true?
1. varargs->limit means varargs->numberOfArgsPassedByCallerIncludingThis.
2. varargs->mandatoryMinimum means varargs->numberOfArgsRequiredByCallee.

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:510
> +                            if (loadIndex + 1 < inlineCallFrame->arguments.size()) {

From reading the code, I see that inlineCallFrame->arguments.size is set to be the argumentsCountIncludingThis of the callee function to be inlined in ByteCodeParser::InlineStackEntry::InlineStackEntry().  Hence, I expect its indexes to range from 0 (for this) to N for the Nth argument.  How is it that you compare it to loadIndex + 1 here where loadIndex can have a varargsData->offset adjustment which comes from the caller?  

I’m sure I’m reading something wrong.  Can you explain what I’m misinterpreting here please?

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 253578 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253578&action=review
> 
> > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:501
> > +                        DFG_ASSERT(m_graph, node, varargsData->limit - 1 >= varargsData->mandatoryMinimum);
> > +                        unsigned limit = std::max(
> > +                            varargsData->limit - 1, varargsData->mandatoryMinimum);
> 
> Is the following true?
> 1. varargs->limit means varargs->numberOfArgsPassedByCallerIncludingThis.

No.

> 2. varargs->mandatoryMinimum means varargs->numberOfArgsRequiredByCallee.

Yes.

> 
> > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:510
> > +                            if (loadIndex + 1 < inlineCallFrame->arguments.size()) {
> 
> From reading the code, I see that inlineCallFrame->arguments.size is set to
> be the argumentsCountIncludingThis of the callee function to be inlined in
> ByteCodeParser::InlineStackEntry::InlineStackEntry().  Hence, I expect its
> indexes to range from 0 (for this) to N for the Nth argument.  How is it
> that you compare it to loadIndex + 1 here where loadIndex can have a
> varargsData->offset adjustment which comes from the caller?  

This entire code is guarded by:

                    if (inlineCallFrame
                        && !inlineCallFrame->isVarargs()
                        && inlineCallFrame->arguments.size() - varargsData->offset <= varargsData->limit) {

We use loadIndex + 1 because loadIndex is an argumentIndex rather than an argumentIndexIncludingThis.

> 
> I’m sure I’m reading something wrong.  Can you explain what I’m
> misinterpreting here please?

[Geoffrey Garen]

Comment on attachment 253578 [details]
the patch

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

>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:501
>> +                            varargsData->limit - 1, varargsData->mandatoryMinimum);
> 
> Is the following true?
> 1. varargs->limit means varargs->numberOfArgsPassedByCallerIncludingThis.
> 2. varargs->mandatoryMinimum means varargs->numberOfArgsRequiredByCallee.

unsigned mandatoryMinimum; // The number of elements on the stack that must be initialized; if the array is too short then the missing elements must get undefined. Does not include "this".
    unsigned limit; // Maximum number of elements to load. Includes "this".

[Geoffrey Garen]

Comment on attachment 253578 [details]
the patch

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

r=me

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:521
> +                                if (storeIndex >= varargsData->mandatoryMinimum) {

I'm curious: When does this case happen, and does it happen in the attached test case? If not, it's probably worth adding a test case for this.

[Filip Pizlo]

(In reply to comment #6)
> Comment on attachment 253578 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253578&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:521
> > +                                if (storeIndex >= varargsData->mandatoryMinimum) {
> 
> I'm curious: When does this case happen, and does it happen in the attached
> test case? If not, it's probably worth adding a test case for this.

This will trigger it:

function foo() { // mandatoryMinimum = 0, since foo->numParameters() = 0
}

function bar() {
    foo.apply(this, arguments); // the arguments object here is statically known to have 3 entries, when we inline this into baz.  In this case, limit will be 4.
}

function baz() {
    bar(1, 2, 3);
}

I'll add a test.

[Filip Pizlo]

Comment on attachment 253578 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:500
> +                        DFG_ASSERT(m_graph, node, varargsData->limit - 1 >= varargsData->mandatoryMinimum);
> +                        unsigned limit = std::max(

I guess I shouldn't use max here, and just use limit - 1 directly, since the above is a release assert.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/184781