Last Comment Bug 621954 - common BindLocalVariable harder in jsparse.cpp
: common BindLocalVariable harder in jsparse.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Brendan Eich [:brendan]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-29 12:03 PST by Brendan Eich [:brendan]
Modified: 2011-07-20 12:57 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (7.32 KB, patch)
2011-07-05 16:45 PDT, Brendan Eich [:brendan]
cdleary: review+
Details | Diff | Splinter Review
patch to commit (7.28 KB, patch)
2011-07-06 16:04 PDT, Brendan Eich [:brendan]
brendan: review+
Details | Diff | Splinter Review

Description Brendan Eich [:brendan] 2010-12-29 12:03:14 PST
Its two call sites look like this:

            uintN index = fun->u.i.nvars;
            if (!BindLocalVariable(context, fun, apn->pn_atom, JSLOCAL_VAR, true))
                return NULL;
            apn->pn_cookie.set(funtc.staticLevel, index);
            apn->pn_dflags |= PND_BOUND;

and

        uintN index = tc->fun()->u.i.nvars;
        if (!BindLocalVariable(cx, tc->fun(), atom, localKind, false))
            return JS_FALSE;
        pn->pn_op = JSOP_GETLOCAL;
        pn->pn_cookie.set(tc->staticLevel, index);
        pn->pn_dflags |= PND_BOUND;

In the first case, pn_op has already been set to JSOP_SETLOCAL much earlier (in BindDestrucuringArg), so the second call site's pn_op setting can stay uncommon.

/be
Comment 1 Brendan Eich [:brendan] 2010-12-29 12:04:43 PST
Tiny bug but I wanted to record it. Anyone cc'ed should feel free to steal, make blocking if this comes up while fixing some other bug, etc.

/be
Comment 2 Brendan Eich [:brendan] 2011-07-05 16:45:12 PDT
Created attachment 544099 [details] [diff] [review]
proposed fix

I pruned a useless arg (varname) and some excessive args (need a separate cleanup patch to eliminate cx, available via tc->parser->context). This passes tests. No functional change, just code sharing.

Picked an unrelated else-after-return nit, and parallelized a nested if to avoid ugly else-if-return.

/be
Comment 3 Brendan Eich [:brendan] 2011-07-06 09:18:22 PDT
Oh, and the bool isArg parameter to BindLocalVariable was crying out for the code it conditioned in that helper to be moved up and out to the only caller that set isArg so as to enable that code: BindFunctionLocal. I beefed up the comment for this 'var arguments'-never-rebinds case to cite NoteLValue.

/be
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-07-06 14:28:42 PDT
Comment on attachment 544099 [details] [diff] [review]
proposed fix

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

::: js/src/frontend/ParseMaps-inl.h
@@ +104,5 @@
>  
>      DefnOrHeader &doh = p.value();
>      if (doh.isHeader())
>          return MultiDeclRange(doh.header());
> +    return MultiDeclRange(doh.defn());

I'm surprised you didn't opt for ternary!

::: js/src/jsparse.cpp
@@ +1903,2 @@
>  {
>      JS_ASSERT(kind == VARIABLE || kind == CONSTANT);

Can we add a comment-assertion along the lines of:

    /* The arguments atom is only bound as a local via destructuring arguments. */
    JS_ASSERT_IF(pn->pn_atom == cx->runtime->atomState.argumentsAtom, kind == VARIABLE);

@@ -3191,2 @@
>                  return NULL;
> -            apn->pn_cookie.set(funtc.staticLevel, index);

Okay, stuff here used to get PND_BOUND in BindDestructuringArg, now we're just doing it twice. No problemo.

@@ +3830,1 @@
>      if (name == cx->runtime->atomState.argumentsAtom) {

Backref anchor.

@@ +3849,5 @@
> +         * Instead, 'var arguments' always restates that predefined binding of
> +         * the lexical environment for function activations. Assignment to such
> +         * a variable must be handled specially -- see NoteLValue.
> +         */
> +        if (name != cx->runtime->atomState.argumentsAtom) {

I'm pretty sure this test is always true, due to the guard above. If so, JS_ASSERT(name != cx->runtime->atomState.argumentsAtom) and hoist the comment.

@@ +4014,5 @@
>  
>      if (tc->inFunction())
> +        return BindFunctionLocal(cx, data, mdl, tc);
> +
> +    return BindTopLevelVar(cx, data, pn, tc);

Thought you were a fan of the ternaries in return position, but it seems like I was wrong!
Comment 5 Brendan Eich [:brendan] 2011-07-06 15:10:08 PDT
(In reply to comment #4)
> Comment on attachment 544099 [details] [diff] [review] [review]
> proposed fix
> 
> Review of attachment 544099 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/ParseMaps-inl.h
> @@ +104,5 @@
> >  
> >      DefnOrHeader &doh = p.value();
> >      if (doh.isHeader())
> >          return MultiDeclRange(doh.header());
> > +    return MultiDeclRange(doh.defn());
> 
> I'm surprised you didn't opt for ternary!

Can't do it here without a cast -- try it and you'll see what I mean.

> ::: js/src/jsparse.cpp
> @@ +1903,2 @@
> >  {
> >      JS_ASSERT(kind == VARIABLE || kind == CONSTANT);
> 
> Can we add a comment-assertion along the lines of:
> 
>     /* The arguments atom is only bound as a local via destructuring
> arguments. */
>     JS_ASSERT_IF(pn->pn_atom == cx->runtime->atomState.argumentsAtom, kind
> == VARIABLE);

Sure, good idea.

> @@ -3191,2 @@
> >                  return NULL;
> > -            apn->pn_cookie.set(funtc.staticLevel, index);
> 
> Okay, stuff here used to get PND_BOUND in BindDestructuringArg, now we're
> just doing it twice. No problemo.

Heading toward doing it once, in some future bug. Not yet filed. I'll get to it.

> @@ +3830,1 @@
> >      if (name == cx->runtime->atomState.argumentsAtom) {
> 
> Backref anchor.

Indeed, with early return in the consequent.

> @@ +3849,5 @@
> > +         * Instead, 'var arguments' always restates that predefined binding of
> > +         * the lexical environment for function activations. Assignment to such
> > +         * a variable must be handled specially -- see NoteLValue.
> > +         */
> > +        if (name != cx->runtime->atomState.argumentsAtom) {
> 
> I'm pretty sure this test is always true, due to the guard above. If so,
> JS_ASSERT(name != cx->runtime->atomState.argumentsAtom) and hoist the
> comment.

Wow, so the isArg test was bogus all along. Thanks!

> @@ +4014,5 @@
> >  
> >      if (tc->inFunction())
> > +        return BindFunctionLocal(cx, data, mdl, tc);
> > +
> > +    return BindTopLevelVar(cx, data, pn, tc);
> 
> Thought you were a fan of the ternaries in return position, but it seems
> like I was wrong!

I left that alone, it didn't seem any better with ?: and (because of the number and difference in type of arguments, and the overall length) arguably worse.

/be
Comment 6 Brendan Eich [:brendan] 2011-07-06 16:04:34 PDT
Created attachment 544367 [details] [diff] [review]
patch to commit
Comment 7 Chris Leary [:cdleary] (not checking bugmail) 2011-07-06 17:06:24 PDT
Comment on attachment 544367 [details] [diff] [review]
patch to commit

Can there be a short comment on the kind == VARIABLE assertion? It wouldn't be obvious to me at first blush.
Comment 8 Brendan Eich [:brendan] 2011-07-07 23:06:56 PDT
Re: comment 7 -- sure, with misplaced only relocated:

  /* The arguments atom is bound as a local only via destructuring arguments. */

In comment 5, I wrote:

> Can't do it here without a cast -- try it and you'll see what I mean.

but that was with ?: inside a single outer MultiDeclRange(). Since that requires a cast after ? or : on the inside to unify the two types, it seemed better to minimize change and leave the if-doh-is-a-header early return case as the special case, and just lose the else after return.

/be
Comment 9 Brendan Eich [:brendan] 2011-07-07 23:12:34 PDT
http://hg.mozilla.org/tracemonkey/rev/a37db4d95026

/be
Comment 10 Brendan Eich [:brendan] 2011-07-08 23:01:22 PDT
http://hg.mozilla.org/tracemonkey/rev/7abfbaac0693 (relanding after TI back-out)

/be
Comment 11 Jason Orendorff [:jorendorff] 2011-07-20 07:22:46 PDT
This never made it into mozilla-central.

Brendan, tracemonkey repo is defunct for now. We're using mozilla-inbound:
http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/d6d7a888b0bee104

I'll rebase and land it now.
Comment 12 Jason Orendorff [:jorendorff] 2011-07-20 08:14:32 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/213e1df84fa4

Note You need to log in before you can comment on or make changes to this bug.