Bug 128850 – GetMyArgumentByVal in FTL

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

[Matthew Mirman]

patch forthcoming.

[Matthew Mirman]

Created attachment 224258 [details]
Added GetMyArgumentByVal to FTL

[WebKit Commit Bot]

Attachment 224258 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2049:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2052:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matthew Mirman]

Created attachment 224261 [details]
Added GetMyArgumentByVal to FTL

Fixed whitespace.

[WebKit Commit Bot]

Comment on attachment 224261 [details]
Added GetMyArgumentByVal to FTL

Clearing flags on attachment: 224261

Committed r164166: <http://trac.webkit.org/changeset/164166>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

This broke three tests:

** The following JSC stress test failures have been introduced:
	regress/script-tests/variadic-closure-call.js.default-ftl
	regress/script-tests/variadic-closure-call.js.ftl-no-cjit-validate
	regress/script-tests/variadic-closure-call.js.ftl-no-cjit-osr-validation
	regress/script-tests/variadic-closure-call.js.ftl-eager
	regress/script-tests/variadic-closure-call.js.ftl-eager-no-cjit
	regress/script-tests/variadic-closure-call.js.ftl-eager-no-cjit-osr-validation
	jsc-layout-tests.yaml/js/script-tests/unmatching-argument-count.js.layout-ftl-eager-no-cjit
	regress/script-tests/direct-arguments-getbyval.js.ftl-eager-no-cjit
	regress/script-tests/direct-arguments-getbyval.js.ftl-eager-no-cjit-osr-validation

I was just doing run-javascriptcore-tests --release --ftl-jit

I'll roll this out and we can look at how to fix it later.

[Filip Pizlo]

Rolled out in http://trac.webkit.org/changeset/164178

[Matthew Mirman]

Created attachment 224432 [details]
Added GetMyArgumentByVal to FTL

Fixed inverted speculation.

[WebKit Commit Bot]

Attachment 224432 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2056:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matthew Mirman]

Created attachment 224439 [details]
Added GetMyArgumentByVal to FTL

Ugh, spaces before newlines.

[Filip Pizlo]

Comment on attachment 224439 [details]
Added GetMyArgumentByVal to FTL

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2052
> +        speculate(Uncountable, noValue(), 0, m_out.isNull(argsPtr.value()));

You're null-checking the address of the stack offset?  Sounds broken. ;-)

[Filip Pizlo]

Comment on attachment 224439 [details]
Added GetMyArgumentByVal to FTL

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2050
> +        speculate(Uncountable, noValue(), 0, 
> +            m_out.aboveOrEqual(propPtr, 
> +                addressFor(m_node->origin.semantic.stackOffset() + JSStack::ArgumentCount).value()));

This is also busted - the ArgumentCount is on the stack only for not-inlined code.

[Filip Pizlo]

(In reply to comment #11)
> (From update of attachment 224439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224439&action=review
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2052
> > +        speculate(Uncountable, noValue(), 0, m_out.isNull(argsPtr.value()));
> 
> You're null-checking the address of the stack offset?  Sounds broken. ;-)

Note that you should probably be doing something like:

- Check that the arguments object had not been allocated.
- Check that you're in bounds, by using either the non-constant length in the call frame (for not-inlined, i.e. outermost, code) or the length constant in the InlineCallFrame (for inlined code).
- If the function's symbol table has SlowArguments then do some other stuff (see DFGSpecualtiveJIT)

Note that the symbolTable having SlowArguments is not the same thing as Arguments having SlowArguments.  symbolTable having SlowArguments indicates that an argument was captured inside of an activation.  You could probably assert that this isn't the case for now since the FTL doesn't yet support activations.

[Matthew Mirman]

Created attachment 224444 [details]
Added GetMyArgumentByVal to FTL

Fixed both speculations.

[Filip Pizlo]

Comment on attachment 224444 [details]
Added GetMyArgumentByVal to FTL

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.

[Matthew Mirman]

Created attachment 224566 [details]
Added GetMyArgumentByVal to FTL and fixed compileGetMyArgumentsLength

the fix for compileGetMyArgumentsLength was similar enough to the addition to GetMyArgumentByVal that I decided to include it here.

[Matthew Mirman]

Created attachment 224567 [details]
Added GetMyArgumentByVal to FTL and fixed compileGetMyArgumentsLength

Whitespace.

[WebKit Commit Bot]

Attachment 224567 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1820:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2055:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2070:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 224567 [details]
Added GetMyArgumentByVal to FTL and fixed compileGetMyArgumentsLength

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1816
> +        if (codeLocation.inlineCallFrame) {

You don't need enclosing { } for one-line control clauses.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1819
> +            TypedPointer reg = addressFor(JSStack::ArgumentCount); 

payloadFor()?  I think your code works for now because the payload is the low 32 bits.  But, it would break if we ever went big endian.  (Not that any sensible person would ever use big endian; little endian is way better.)

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2062
> +                JSStack::ThisArgument * sizeof(Register) + sizeof(Register))));

You don't need this extra offset thingy, the "JSStack::ThisArgument * sizeof...".  In fact, having that makes the code wrong.

Have you run-javascriptcore-tests btw? ;-)

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2069
> +                  m_out.isNull(m_out.loadPtr(m_out.address(m_heaps.variables.atAnyIndex(), m_callFrame, Arguments::offsetOfSlowArgumentData()))));

Shouldn't you be exiting if the slow argument data is non-null, rather than when it's null?  Also, I don't understand what you're loading here.  You seem to be loading the frame pointer from the stack, which is indeed never null, and also has nothing to do with slow arguments.

[Matthew Mirman]

Created attachment 224911 [details]
Added GetMyArgumentByVal to FTL

Ran run-javascriptcore-tests release --ftl-jit and got no failures.

[WebKit Commit Bot]

Attachment 224911 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2078:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2079:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #21)
> Attachment 224911 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2078:  Tab found; better to use spaces  [whitespace/tab] [1]

You should definitely fix this and upload a new patch.

> ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2079:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Total errors found: 2 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 224911 [details]
Added GetMyArgumentByVal to FTL

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

This looks good to me, but I would fix the indentation/style goofs.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2044
> +		LValue property = lowInt32(m_node->child1());

Weird indentation.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2045
> +        CodeOrigin& codeLocation = m_node->origin.semantic;

I wouldn't use a reference here.  There's no point since it's a small struct and probably copying it from the heap is faster than having a pointer to it.  It's also somewhat safer in general.  I mean, in this particular case, nobody will delete the thing that m_node points to, but in general I like to pass-by-value when I can because it means I don't have to worry about object lifecycles as much.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2050
> +        if (!isEmptySpeculation(m_state.variables().operand(m_graph.argumentsRegisterFor(codeLocation)).m_type))
> +            speculate(ArgumentsEscaped, noValue(), 0,
> +                m_out.notNull(m_out.loadPtr(addressFor(m_graph.machineArgumentsRegisterFor(codeLocation)))));

Multi-line control clauses need { }.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2059
> +        if (inlineCallFrame)
> +            speculate(Uncountable, noValue(), 0, 
> +                m_out.aboveOrEqual(property,
> +                    m_out.constInt32(inlineCallFrame->arguments.size() - 1)));
> +        else 
> +            speculate(Uncountable, noValue(), 0, 
> +                m_out.aboveOrEqual(m_out.add(property, m_out.constInt32(1)), 
> +                    m_out.load32NonNegative(payloadFor(JSStack::ArgumentCount))));

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2063
> +            RELEASE_ASSERT(false); // TODO

RELEASE_ASSERT_NOT_REACHED().  Good form would be to file a bug on bugzilla for the part that needs to be done.  It can be a super terse bug, like title = "FTL GetArgumentByVal should handle the slowArguments case", and description = "..." (I use that to have a non-empty string in the description to make bugzilla happy.)

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2070
> +        setJSValue(m_out.load64(m_out.baseIndex(m_heaps.variables.atAnyIndex(),
> +            inlineCallFrame ? addressFor(inlineCallFrame->arguments[0].virtualRegister()).value() : m_callFrame,
> +            m_out.signExt(property, m_out.intPtr), ScaleEight, 
> +            offsetOfArguments(inlineCallFrame))));
> +    }

This looks suspicious in case of inlined code.  You're first getting the addressFor() arguments[0], and then you're putting in an offset which again calculates the offset of arguments[1].  Both of these calculations will contain almost the same offset.  This seems wrong.  It'll work for not-inlined code since you use m_callFrame as a base and then put in the argumentsOffset of the zero'th argument.  But for inlined code you're adding something like that twice.  Also, inlineCallFrame->arguments[0] could be unset, leading to a crash.

I think that you should try to rearchitect this so that you're first computing the address of the zero'th argument and then passing that as the base of baseIndex, and then you'll use zero as the offset (i.e. you won't pass the offset).  The helper should therefore be addressOfArguments rather than offsetOfArguments.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2081
> +    int offsetOfArguments(InlineCallFrame* inlineCallFrame)
> +    {
> +        if (!inlineCallFrame)
> +            return CallFrame::argumentOffset(0) * sizeof(Register);
> +        if (inlineCallFrame->arguments.size() <= 1)
> +            return 0;
> +        ValueRecovery recovery = inlineCallFrame->arguments[1];
> +        RELEASE_ASSERT(recovery.technique() == DisplacedInJSStack);
> +        return recovery.virtualRegister().offset() * sizeof(Register);
> +    }

Move this below all of the compileBlahBlah method definitions.  Also, I think the convention we usually use is that we pass around VirtualRegisters right up until the bitter end.  So, you could call this registerOfBaseArgument or something.  It would be the same as what you did except it would return a VirtualRegister and it wouldn't be a multiple of sizeof(Register).  But, I think it would be even better if you just made a helper called addressOfArguments that returns a LValue that is the address of the zero'th argument.

[Filip Pizlo]

(In reply to comment #23)
> (From update of attachment 224911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224911&action=review
> 
> This looks good to me, but I would fix the indentation/style goofs.

Heh, correction, I did find a bug.  For inlined code you're adding the same offset twice.  Can you write a test for the inlining case and verify by looking at the IR that inlining happened?  It would probably be something like:

function foo(i) {
    return arguments[i];
}

function bar(i) {
    return foo(i, 89, 23, 54);
}

noInline(bar);

for (var i = 0; i < 100000; ++i) {
    var result = bar(1 + (i % 3));
    if (result != [89, 23, 54][i % 3])
        throw "Error: bad result for i = " + i + ": " + result;
}

Notice how I use bar as the function that isn't inlined and that will ultimately be compiled by the FTL, but foo is the thing that uses arguments.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2044
> > +		LValue property = lowInt32(m_node->child1());
> 
> Weird indentation.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2045
> > +        CodeOrigin& codeLocation = m_node->origin.semantic;
> 
> I wouldn't use a reference here.  There's no point since it's a small struct and probably copying it from the heap is faster than having a pointer to it.  It's also somewhat safer in general.  I mean, in this particular case, nobody will delete the thing that m_node points to, but in general I like to pass-by-value when I can because it means I don't have to worry about object lifecycles as much.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2050
> > +        if (!isEmptySpeculation(m_state.variables().operand(m_graph.argumentsRegisterFor(codeLocation)).m_type))
> > +            speculate(ArgumentsEscaped, noValue(), 0,
> > +                m_out.notNull(m_out.loadPtr(addressFor(m_graph.machineArgumentsRegisterFor(codeLocation)))));
> 
> Multi-line control clauses need { }.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2059
> > +        if (inlineCallFrame)
> > +            speculate(Uncountable, noValue(), 0, 
> > +                m_out.aboveOrEqual(property,
> > +                    m_out.constInt32(inlineCallFrame->arguments.size() - 1)));
> > +        else 
> > +            speculate(Uncountable, noValue(), 0, 
> > +                m_out.aboveOrEqual(m_out.add(property, m_out.constInt32(1)), 
> > +                    m_out.load32NonNegative(payloadFor(JSStack::ArgumentCount))));
> 
> Ditto.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2063
> > +            RELEASE_ASSERT(false); // TODO
> 
> RELEASE_ASSERT_NOT_REACHED().  Good form would be to file a bug on bugzilla for the part that needs to be done.  It can be a super terse bug, like title = "FTL GetArgumentByVal should handle the slowArguments case", and description = "..." (I use that to have a non-empty string in the description to make bugzilla happy.)
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2070
> > +        setJSValue(m_out.load64(m_out.baseIndex(m_heaps.variables.atAnyIndex(),
> > +            inlineCallFrame ? addressFor(inlineCallFrame->arguments[0].virtualRegister()).value() : m_callFrame,
> > +            m_out.signExt(property, m_out.intPtr), ScaleEight, 
> > +            offsetOfArguments(inlineCallFrame))));
> > +    }
> 
> This looks suspicious in case of inlined code.  You're first getting the addressFor() arguments[0], and then you're putting in an offset which again calculates the offset of arguments[1].  Both of these calculations will contain almost the same offset.  This seems wrong.  It'll work for not-inlined code since you use m_callFrame as a base and then put in the argumentsOffset of the zero'th argument.  But for inlined code you're adding something like that twice.  Also, inlineCallFrame->arguments[0] could be unset, leading to a crash.
> 
> I think that you should try to rearchitect this so that you're first computing the address of the zero'th argument and then passing that as the base of baseIndex, and then you'll use zero as the offset (i.e. you won't pass the offset).  The helper should therefore be addressOfArguments rather than offsetOfArguments.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2081
> > +    int offsetOfArguments(InlineCallFrame* inlineCallFrame)
> > +    {
> > +        if (!inlineCallFrame)
> > +            return CallFrame::argumentOffset(0) * sizeof(Register);
> > +        if (inlineCallFrame->arguments.size() <= 1)
> > +            return 0;
> > +        ValueRecovery recovery = inlineCallFrame->arguments[1];
> > +        RELEASE_ASSERT(recovery.technique() == DisplacedInJSStack);
> > +        return recovery.virtualRegister().offset() * sizeof(Register);
> > +    }
> 
> Move this below all of the compileBlahBlah method definitions.  Also, I think the convention we usually use is that we pass around VirtualRegisters right up until the bitter end.  So, you could call this registerOfBaseArgument or something.  It would be the same as what you did except it would return a VirtualRegister and it wouldn't be a multiple of sizeof(Register).  But, I think it would be even better if you just made a helper called addressOfArguments that returns a LValue that is the address of the zero'th argument.

[Filip Pizlo]

Created attachment 225601 [details]
the patch

[Filip Pizlo]

Comment on attachment 225601 [details]
the patch

This revealed bugs.  I will have a patch and more tests up shortly.

[Filip Pizlo]

Created attachment 225604 [details]
the patch

[Filip Pizlo]

Comment on attachment 225604 [details]
the patch

Not yet ready for review.

[Geoffrey Garen]

Comment on attachment 225604 [details]
the patch

r=me

[Filip Pizlo]

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