724790 - can we avoid this hasOverriddenArgs funny business?

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

If I understand correctly, 'arguments' can only be overwritten by a direct assignment in a function or a direct eval in a function.  In particular, 'f.arguments = 3' does not overwrite arguments.  In that case, it seems like we should just stop emitting jsop_arguments (emitting name ops instead) and avoid the need to test for overwritten arguments.

Comment 1

[Luke Wagner [:luke]]

Attachment: rm overwritten args

We already track when we overwrite 'arguments' in a formal so it is easy enough to add the symmetric case for overwritten locals.  The patch as is, I think, a strict simplification of the current code, but not much.  However, this pays off with my later patch to make arguments (mostly) eager in a way that ultimately allows GetCallArguments/SetCallArguments/call_resolve to be replaced by a simple data property.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 595232 [details] [diff] [review]
rm overwritten args

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

::: js/src/frontend/BytecodeEmitter.h
@@ +167,5 @@
>  #define TCF_RETURN_VOID         0x08 /* function has 'return;' */
>  #define TCF_IN_FOR_INIT         0x10 /* parsing init expr of for; exclude 'in' */
>  #define TCF_FUN_SETS_OUTER_NAME 0x20 /* function set outer name (lexical or free) */
>  #define TCF_FUN_PARAM_ARGUMENTS 0x40 /* function has parameter named arguments */
> +#define TCF_FUN_LOCAL_ARGUMENTS 0x80 /* function has local named arguments */

TCF_FUN_ARGUMENTS_LOCAL reads better to me, as in "has an arguments local".  The other ordering suggests the arguments are local, which is true -- but vacuously so.  Although this doesn't strike me as the best possible name either...

@@ -183,2 @@
>                                          from global script */
> -/* bit 0x8000 is unused */

Sigh, I hate keeping long lists of bit names in order.

@@ +271,5 @@
>   */
>  #define TCF_FUN_FLAGS           (TCF_FUN_SETS_OUTER_NAME |                    \
>                                   TCF_FUN_USES_ARGUMENTS  |                    \
>                                   TCF_FUN_PARAM_ARGUMENTS |                    \
> +                                 TCF_FUN_LOCAL_ARGUMENTS |                    \

Should this really be the case?  I wouldn't think so, given every function has its own unique |arguments|.

@@ +437,5 @@
> +    bool mayOverwriteArguments() const {
> +        JS_ASSERT(inFunction());
> +        return !inStrictMode() &&
> +               (callsEval() ||
> +                flags & (TCF_FUN_PARAM_ARGUMENTS | TCF_FUN_LOCAL_ARGUMENTS));

We might as well assert that if inStrictMode() those flags aren't present.

::: js/src/frontend/Parser.cpp
@@ +4422,5 @@
>  
>              if (tc->inFunction() && name == context->runtime->atomState.argumentsAtom) {
>                  tc->noteArgumentsNameUse(pn2);
>                  if (!blockObj)
> +                    tc->flags |= (TCF_FUN_HEAVYWEIGHT | TCF_FUN_LOCAL_ARGUMENTS);

So we're going to (already do?) deoptimize functions with a let-variable named |arguments|?  Fine by me.

::: js/src/jsfun.cpp
@@ +672,1 @@
>              callobj.initArguments(ObjectValue(fp->argsObj()));

Looks like initArguments should take ArgumentsObject&...

@@ +759,5 @@
>  {
>      CallObject &callobj = obj->asCall();
>  
>      StackFrame *fp = callobj.maybeStackFrame();
> +    if (fp && callobj.arguments().isUndefined()) {

SetCallArguments will cheerfully set arguments() to |undefined|, so I don't think this works.

[jwalden@wheres-wally src]$ dbg/js
js> function f() { var arguments; arguments = undefined; return arguments; } f(1, 2, 3)
({0:1, 1:2, 2:3})

Which just might blow this idea out of the water, actually.  But you're the one proposing a simplification, and I'm the one poking holes in it, so maybe you see a clever trick to keep things working.

::: js/src/methodjit/StubCalls.cpp
@@ +1368,2 @@
>      f.regs.sp++;
> +    JSObject *arguments = js_GetArgsObject(f.cx, f.fp());

As a reader nicety, it seems better if the variable type here is the narrowest thing possible, i.e. ArgumentsObject*.

::: js/src/vm/ScopeObject.h
@@ +132,5 @@
>      inline JSFunction *getCalleeFunction() const;
>      inline void setCallee(JSObject *callee);
>  
>      /* Returns the callee's arguments object. */
> +    inline const js::Value &arguments() const;

This comment's a lie (now?) -- the value returned is whatever the value of |arguments| is inside the function.

/* Returns the value of the |arguments| binding for this CallObject. */

Maybe?

The way this returns undefined before initing/setting is also a bit weird, and in need of elucidation.  But see below.

@@ +137,2 @@
>      inline void setArguments(const js::Value &v);
>      inline void initArguments(const js::Value &v);

set/init and when to use them are a bit confusing, seems to me.  I am going to assume your subsequent patch simplifying arguments to a data property will take care of this.

::: js/src/vm/Stack.h
@@ +376,1 @@
>      };

This enum needs an entry for the free bits available for future use, if you're going to touch this.

Comment 3

[Luke Wagner [:luke]]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #2)

> >  #define TCF_FUN_FLAGS           (TCF_FUN_SETS_OUTER_NAME |                    \
> >                                   TCF_FUN_USES_ARGUMENTS  |                    \
> >                                   TCF_FUN_PARAM_ARGUMENTS |                    \
> > +                                 TCF_FUN_LOCAL_ARGUMENTS |                    \
> 
> Should this really be the case?  I wouldn't think so, given every function
> has its own unique |arguments|.

Initially I left it out for the same reason but, iirc, this mask seemed to be applied when copying flags from the body of a function to the emitter for the function so I was losing the flag.

> SetCallArguments will cheerfully set arguments() to |undefined|, so I don't
> think this works.

D'oh, I saw this, but I put the fix in a later patch (http://hg.mozilla.org/users/lwagner_mozilla.com/inbound-patches/file/a74e2b978ca9/eager-args-obj#l941) but I didn't push the fix up to this patch.  Good eye; new patch when I get back.

Comment 4

[Luke Wagner [:luke]]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #2)
> >  #define TCF_FUN_PARAM_ARGUMENTS 0x40 /* function has parameter named arguments */
> > +#define TCF_FUN_LOCAL_ARGUMENTS 0x80 /* function has local named arguments */
> 
> TCF_FUN_ARGUMENTS_LOCAL reads better to me, as in "has an arguments local". 

TCF_FUN_LOCAL_ARGUMENTS is symmetric with TCF_FUN_PARAM_ARGUMENTS

Comment 5

[Luke Wagner [:luke]]

Attachment: rm overwritten args v.2

Comment 6

[Luke Wagner [:luke]]

Green on try

Comment 7

[Luke Wagner [:luke]]

Attachment: rm overwritten args v.2 (v.2)

Somehow I managed to re-upload the original patch last time...

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 601050 [details] [diff] [review]
rm overwritten args v.2 (v.2)

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

::: js/src/jit-test/tests/basic/testOverwrittenArgumentsWithUndefined.js
@@ +6,5 @@
> +    eval("assertEq(arguments, undefined)");
> +    arguments = a;
> +    eval("assertEq(arguments[0], 42)");
> +    eval("assertEq(arguments, a)");
> +}

Tangentially:

Does the test pass if you change |f()| to |f(z)| and assign 17 to |z| while |arguments === undefined|, then test for |a[0] === 17|?  Add that in the test if it passes.  (I'm not sure if it does.)  Also add the converse, setting |a[0] = 8675309| while |arguments === undefined|, then testing for |z === 8675309|, if that passes.

::: js/src/jsinterp.cpp
@@ +3014,5 @@
>      if (cx->typeInferenceEnabled() && !script->strictModeCode) {
>          if (!script->ensureRanInference(cx))
>              goto error;
>          if (script->createdArgs) {
> +            JSObject *arguments = js_GetArgsObject(cx, regs.fp());

ArgumentsObject

@@ +3022,5 @@
>          } else {
>              rval = MagicValue(JS_LAZY_ARGUMENTS);
>          }
>      } else {
> +        JSObject *arguments = js_GetArgsObject(cx, regs.fp());

ArgumentsObject

::: js/src/vm/Stack.h
@@ +376,1 @@
>      };

This still needs an entry indicating which bits are free.

Comment 9

[Luke Wagner [:luke]]

Free bits left as followup since there was no consensus on irc as to whether this was actually a good thing.

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

Comment 10

[Matt Brubeck (:mbrubeck)]

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