The default bug view has changed. See this FAQ.

common BindLocalVariable harder in jsparse.cpp

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
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
Assignee: general → brendan
(Assignee)

Comment 2

6 years ago
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
Attachment #544099 - Flags: review?(cdleary)
(Assignee)

Comment 3

6 years ago
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 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!
Attachment #544099 - Flags: review?(cdleary) → review+
(Assignee)

Comment 5

6 years ago
(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
(Assignee)

Comment 6

6 years ago
Created attachment 544367 [details] [diff] [review]
patch to commit
Attachment #544099 - Attachment is obsolete: true
Attachment #544367 - Flags: review+
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.
(Assignee)

Comment 8

6 years ago
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
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/tracemonkey/rev/a37db4d95026

/be
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/tracemonkey/rev/7abfbaac0693 (relanding after TI back-out)

/be
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/213e1df84fa4
http://hg.mozilla.org/mozilla-central/rev/213e1df84fa4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.