740446 - fix overwritten-arguments handling in the front-end

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on mozilla-central revision 7ed31daf07bd with patch from bug 740259 (options -m -n -a):


function f() {
    return++ arguments;
    function arguments() {}
}
assertEq(f(), "object");

Comment 1

[Luke Wagner [:luke]]

Ack.  Overwritten-arguments handling in the frontend is mental.  My patches are just the first thing that assert about it hard enough (and require it to be correct).  I need to do major violence.  Might as well make this the bug to do it.

Comment 3

[Luke Wagner [:luke]]

Attachment: fix arguments

So here's the basic strategy:
 - At parse-time, there is no special designation for 'arguments', its uses become lexdeps and redeclarations get normal local bindings etc.
 - When we finish parsing a function body (end of Parser::functionBody), if 'arguments' is a lexdep (unresolved use), we create a binding.  If there is any dynamic name usage (using bindingsAccessedDynamically; this patch applies atop attachment 610444 [details] [diff] [review] in bug 740259), we also create a binding.  Lastly, if 'arguments' ends up being bound as a local (as commented in the patch, not a parameter!), we set a bit (on emitter and later script) argumentsHasLocalBinding.  When this is true, the value of 'arguments' is in script->localSlot(script->argumentsLocalSlot()).
 - We also make an eager determination "definitelyNeedsArgsObj" in functionBody when (for various reasons) we know an arguments object is definitely necessary and analyzeSSA won't know about it.
 - When emitting a function, we generate a triplet "arguments;setlocal;pop" where 'arguments' either creates/evaluates-to a real arguments object or a magic value indicate "an optimized-away arguments object"
 - What used to be JSOP_ARGUMENTS is now just a JSOP_GETLOCAL from the script->argumentsLocalSlot().

The patch is green on try.  It also removes ~400 lines, CallObject's resolve hook and the ARGUMENTS_SLOT reserved property.  Lastly, the patch (since I had to frob it anyway) generalizes *and* significantly simplifies the f.apply(null, arguments) optimization in the mjit: now any SSA-equivalent pattern, such as 'var a = arguments; f.apply(null, a)' gets optimized, not just when 'arguments' is the literal parameter.

Waldo: could you review the frontend and VM changes?
Brian: could you review the TI and mjit changes?
Jason: could you review the removal of two debugger tests?  Comments welcome on frontend, though.

I had to remove the debugger tests because they tested whether eval-in-frame could see an otherwise-undeclared 'arguments' binding.  Right now, the name lookup just fails to find anything so you get 'undefined'.  With bug 659577, we'll have debug scope proxies so we can do something friendly like throw an exception.

Comment 4

[Luke Wagner [:luke]]

(Patch also fixes existing correctness bugs involving 'arguments'.)

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 612623 [details] [diff] [review]
fix arguments

Instead of deleting the debugger tests, just cut them back to the bare minimum to check that trying to get |arguments| through the debugger does not crash.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 612623 [details] [diff] [review]
fix arguments

Review of attachment 612623 [details] [diff] [review]:
-----------------------------------------------------------------

I'd have put the numArgs/numVars naming changes in a separate rubberstampable patch to cut down on diff size.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1552,5 @@
>               * even if their main effect (their return value) is discarded.
>               *
>               * PNK_LB binary trees of 3 or more nodes are flattened into lists
>               * to avoid too much recursion.  All such lists must be presumed
>               * to be useful because each index operation could invoke a getter

Needs a period.

@@ +1678,2 @@
>                  /*
> +                 * Not a use of a unshadowed named function expression's given

an unshadowed

@@ +1686,5 @@
>          if (pn->isKind(PNK_DOT)) {
> +            /*
> +             * Any dotted property reference could call a getter, except
> +             * for arguments.length where arguments is unambiguous.
> +             */

Seems like this should just be /* Dotted property references in general can call getters. */ without the now-irrelevant arguments bits.

::: js/src/frontend/BytecodeEmitter.h
@@ +466,4 @@
>      }
>  
> +    unsigned argumentsLocalSlot() const {
> +        JSAtom *arguments = parser->context->runtime->atomState.argumentsAtom;

PropertyName, please -- a little more precise, will probably make my life slightly easier soon.

::: js/src/frontend/ParseNode.h
@@ +733,5 @@
>      static ParseNode *
>      newBinaryOrAppend(ParseNodeKind kind, JSOp op, ParseNode *left, ParseNode *right,
>                        TreeContext *tc);
>  
> +    inline JSAtom *atom() const;

PropertyName* please.  (Since these all derived from script, they're contextually required not to be numeric.)

::: js/src/frontend/Parser.cpp
@@ +645,5 @@
>       */
> +    if (!CheckStrictParameters(context, tc))
> +        return NULL;
> +
> +    JSAtom * const arguments = context->runtime->atomState.argumentsAtom;

PropertyName

@@ -6635,5 @@
> -        name == context->runtime->atomState.argumentsAtom)
> -    {
> -        /*
> -         * Bind early to JSOP_ARGUMENTS to relieve later code from having
> -         * to do this work (new rule for the emitter to count on).

New rule: NO RULE.  \o/

::: js/src/jit-test/tests/basic/testBug741497.js
@@ +1,3 @@
> +"use strict";
> +function inner()
> +([arguments, b] = this, c)();

The formatting here is all screwy to read, what with function closuring and misleading newlining.  Put some braces and a return statement around that?

::: js/src/jsscript.cpp
@@ +628,5 @@
>              script->strictModeCode = true;
>          if (scriptBits & (1 << ContainsDynamicNameAccess))
>              script->bindingsAccessedDynamically = true;
> +        if (scriptBits & (1 << ArgumentsHasLocalBinding)) {
> +            JSAtom *arguments = cx->runtime->atomState.argumentsAtom;

PropertyName

::: js/src/jsscript.h
@@ +520,5 @@
>                                 uint16_t nClosedArgs, uint16_t nClosedVars, uint32_t nTypeSets,
>                                 JSVersion version);
>      static JSScript *NewScriptFromEmitter(JSContext *cx, js::BytecodeEmitter *bce);
>  
> +    /* See TCF_ARGUMENTS_HAS_LOCAL_BINDING comment */

Period?

::: js/src/vm/ScopeObject.cpp
@@ -414,5 @@
> -     * arguments object reference in a Call prototype's |arguments| slot.
> -     *
> -     * Include JSPROP_ENUMERATE for consistency with all other Call object
> -     * properties; see js::Bindings::add and js::Interpret's JSOP_DEFFUN
> -     * rebinding-Call-property logic.

Oh man am I happy to see this go.

Comment 7

[Luke Wagner [:luke]]

> I'd have put the numArgs/numVars naming changes in a separate
> rubberstampable patch to cut down on diff size.

You're right, sorry for that.

Comment 8

[Luke Wagner [:luke]]

Attachment: fix arguments v.2

Re-requesting jorendorff for debugger test changes.
Still requesting TI+mjit changes from bhackett.
Carrying forward r+ from Waldo.

Adding dherman to review changes to genexp tests.  I made two changes:
 1. in some cases the error message changed from "SyntaxError: illegal use of arguments in generator expression" to "SyntaxError: generator expression must be parenthesized" (when both errors existed).
 2. more meaningfully: my patch does away with the 30 places we sniff argumentsAtom by instead sniffing lexdeps at the end.  However, there is one place this misses: (1 for (arguments in [])).  Here, 'arguments' is bound to a desugared let block.  In strict mode, this is of course an error (binding 'arguments').  In lenient mode, it doesn't show up in lexdeps so I don't catch it.  However, this doesn't seem like a real error: 'arguments' is not referring to the enclosing function's arguments, it is being bound in a let-for-in statement.  dherman agreed on irc (just repeating logic here) so I actually got to change the test-case and keep the mercifully-simpler parsing logic.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 613062 [details] [diff] [review]
fix arguments v.2

Review of attachment 613062 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3447,2 @@
>          JSOp op;
> +        op = pn2->getOp();

Same line

::: js/src/jit-test/tests/debug/Frame-eval-08.js
@@ +1,4 @@
> +// Test that 'arguments' access in a function that doesn't expect 'arguments'
> +// doesn't crash.
> +
> +// TODO: the debugger should be improved to throw an error in such cases rather

File a bug?

::: js/src/jsinterp.cpp
@@ +2977,5 @@
>  BEGIN_CASE(JSOP_ARGUMENTS)
> +    if (script->needsArgsObj()) {
> +        ArgumentsObject *obj = ArgumentsObject::create(cx, regs.fp());
> +        if (!obj)
> +            return false;

I know nothing about this stuff, but did you want |goto error;|?

::: js/src/jsscript.h
@@ -408,5 @@
>                                   * or has had backedges taken. Reset if the
>                                   * script's JIT code is forcibly discarded. */
> -#if JS_BITS_PER_WORD == 32
> -    void *padding_;
> -#endif

Related?

::: js/src/methodjit/Compiler.cpp
@@ +3983,5 @@
> +     */
> +    interruptCheckHelper();
> +
> +    FrameEntry *origCallee = frame.peek(-(argc + 2));
> +    FrameEntry *origThis = frame.peek(-(argc + 1));

Will this not complain about negating an unsigned integer?

Comment 10

[Luke Wagner [:luke]]

(In reply to Ms2ger from comment #9)
> Same line

Can't, 'goto crosses initialization'

> File a bug?

Have one (bug 659577).  But I'll mention it in the comment.

> I know nothing about this stuff, but did you want |goto error;|?

Good catch; cut/paste is dangerous.

> Related?

Yes.

> Will this not complain about negating an unsigned integer?

No, - on unsigned is well-defined, as is signed-to-unsigned conversion.  I'll add an explicit cast though.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 613062 [details] [diff] [review]
fix arguments v.2

Thanks, luke.

Comment 12

[Dave Herman [:dherman]]

Comment on attachment 613062 [details] [diff] [review]
fix arguments v.2

Updated tests look good.

Dave

Comment 13

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa24b215d49e

Comment 14

[Nicholas Nethercote [inactive]]

You were able to add new fields to JSScript without increasing its size, thanks to the existing padding.  But that is going to undo the gains of some of my JSScript size reduction patches in bug 739512.  It's not grounds for backing this patch out or anything, but still, a bummer :(

Comment 15

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> but still, a bummer :(

Actually, pretty much everyone I told about this patch was delighted.
http://www.youtube.com/watch?v=bQ_LHyx2odc

> It's not grounds for backing this patch out or anything

Let's see: 4 bytes, that's 2% of sizeof(JSScript) which, by my previous measurements, is about half of js-main-runtime-scripts which itself is about 9% of total explicit.  So, that's .09% total explicit usage.  I should hope it's not grounds for backing out.

Lastly, I have to say it again: bug 678037 makes sizeof(JSScript) matter a whole lot less; I think that is the real path to harmony here.

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/fa24b215d49e