Closed Bug 740446 Opened 10 years ago Closed 10 years ago

fix overwritten-arguments handling in the front-end


(Core :: JavaScript Engine, defect)

Not set





(Reporter: decoder, Assigned: luke)



(Keywords: assertion, testcase)


(2 files)

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");
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.
Summary: ALIASEDVAR Patch: Assertion failure: script->varIsAliased(i), at vm/ScopeObject.cpp:366 → fix overwritten-arguments handling in the front-end
Duplicate of this bug: 741497
Attached patch fix argumentsSplinter Review
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.
Attachment #612623 - Flags: review?(jwalden+bmo)
Attachment #612623 - Flags: review?(jorendorff)
Attachment #612623 - Flags: review?(bhackett1024)
(Patch also fixes existing correctness bugs involving 'arguments'.)
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.
Attachment #612623 - Flags: review?(jorendorff)
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;


@@ -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;


::: 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 */


::: 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.
Attachment #612623 - Flags: review?(jwalden+bmo) → review+
> 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.
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.
Attachment #613062 - Flags: review?(jorendorff)
Attachment #613062 - Flags: review?(dherman)
Attachment #613062 - Flags: review?(bhackett1024)
Attachment #612623 - Flags: review?(bhackett1024)
Attachment #613062 - Flags: review?(bhackett1024) → review+
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 @@
> +    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


::: 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?
(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?


> 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 on attachment 613062 [details] [diff] [review]
fix arguments v.2

Thanks, luke.
Attachment #613062 - Flags: review?(jorendorff) → review+
Comment on attachment 613062 [details] [diff] [review]
fix arguments v.2

Updated tests look good.

Attachment #613062 - Flags: review?(dherman) → review+
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 :(
(In reply to Nicholas Nethercote [:njn] from comment #14)
> but still, a bummer :(

Actually, pretty much everyone I told about this patch was delighted.

> 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.
Closed: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 530650
Duplicate of this bug: 574834
Duplicate of this bug: 604251
Blocks: 743876
Depends on: 746601
Duplicate of this bug: 737385
You need to log in before you can comment on or make changes to this bug.