Bug 129051 – GetMyArgumentsByLength in FTL doesn't work with inline code

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

[Matthew Mirman]

patch forthcoming.

[Matthew Mirman]

Created attachment 224651 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

[Geoffrey Garen]

Comment on attachment 224651 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Can you add a test case for this? (If it's already covered by an existing test, you should note that in your ChangeLog.)

[Matthew Mirman]

Created attachment 224654 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Added a test case.

[Geoffrey Garen]

Comment on attachment 224654 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

dfgShouldBe is a slightly better way to write a test like this. It tests precisely whether the function made it into our optimizing compilers, which means the test can terminate sooner, and there's no risk that the test will terminate too soon. You should make that change in a follow-up patch.

[WebKit Commit Bot]

Comment on attachment 224654 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Rejecting attachment 224654 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 224654, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
kitpy/common/checkout/changelog.py", line 347, in parse_entries_from_file
    yield ChangeLogEntry(''.join(entry_lines[:-1]), revision=most_probable_revision)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 112, in __init__
    self._parse_entry()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 206, in _parse_entry
    self._date_line = match.group()
AttributeError: 'NoneType' object has no attribute 'group'

Full output: http://webkit-queues.appspot.com/results/5252063646711808

[Filip Pizlo]

Comment on attachment 224654 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Actually, this test is broken.  There's no way it even hits the FTL since it relies on global code.  Did you test the test?  It's very important that when you add a test, you check that it indeed reproduces the bug you're trying to fix.  I don't buy that this test gives us coverage over the old version of this code.

[Matthew Mirman]

Created attachment 224890 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

[Filip Pizlo]

Comment on attachment 224890 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:300
> +        case GetMyArgumentsLengthSafe:

I don't think you want to combine these two.  The purpose of GetMyArgumenstLengthSafe is to have a slow path, rather than a speculation, for the arguments-created case.  See what the DFG does, notice the slow path call:

    case GetMyArgumentsLengthSafe: {
        GPRTemporary result(this);
        GPRReg resultGPR = result.gpr();
        
        JITCompiler::Jump created = m_jit.branchTest64(
            JITCompiler::NonZero,
            JITCompiler::addressFor(
                m_jit.graph().machineArgumentsRegisterFor(node->origin.semantic)));
        
        if (node->origin.semantic.inlineCallFrame) {
            m_jit.move(
                Imm64(JSValue::encode(jsNumber(node->origin.semantic.inlineCallFrame->arguments.size() - 1))),
                resultGPR);
        } else {
            m_jit.load32(JITCompiler::payloadFor(JSStack::ArgumentCount), resultGPR);
            m_jit.sub32(TrustedImm32(1), resultGPR);
            m_jit.or64(GPRInfo::tagTypeNumberRegister, resultGPR);
        }
        
        // FIXME: the slow path generator should perform a forward speculation that the
        // result is an integer. For now we postpone the speculation by having this return
        // a JSValue.
        
        addSlowPathGenerator(
            slowPathCall(
                created, this, operationGetArgumentsLength, resultGPR,
                m_jit.graph().machineArgumentsRegisterFor(node->origin.semantic).offset()));
        
        jsValueResult(resultGPR, node);
        break;
    }

[Matthew Mirman]

Created attachment 224894 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

[WebKit Commit Bot]

Comment on attachment 224894 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Clearing flags on attachment: 224894

Committed r164496: <http://trac.webkit.org/changeset/164496>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.