Last Comment Bug 775323 - build Bindings after, not during, parsing
: build Bindings after, not during, parsing
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 772328 775684 775734 778919 784550
Blocks: 767013
  Show dependency treegraph
 
Reported: 2012-07-18 15:00 PDT by Luke Wagner [:luke]
Modified: 2012-08-21 16:59 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move bindings construction (2.20 KB, patch)
2012-08-01 15:38 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Review
create bindings after parsing (160.04 KB, patch)
2012-08-01 15:51 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
rm PN_NAMESET (10.31 KB, patch)
2012-08-01 15:56 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
rm PN_NAMESET (10.32 KB, patch)
2012-08-01 17:00 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
rm PN_NAMESET and dead uses (11.10 KB, patch)
2012-08-01 17:04 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review
combined patch for fuzzing (applies to cset 84ae895140f3) (208.16 KB, patch)
2012-08-02 14:56 PDT, Luke Wagner [:luke]
gary: feedback+
choller: feedback+
Details | Diff | Review
create bindings after parsing (160.04 KB, patch)
2012-08-02 22:48 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
create bindings after parsing (160.89 KB, patch)
2012-08-06 09:15 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
create bindings after parsing (163.21 KB, patch)
2012-08-08 11:47 PDT, Luke Wagner [:luke]
ejpbruel: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2012-07-18 15:00:32 PDT
For bug 767013, we need to only create bindings for aliased variables.  However, we only know this after we finish parsing a function body, so this bug is to build Bindings at the end of Parser::functionBody (or somewhere thereabouts).  This should allow several simplifications:
 - Bindings will be created all at once from TreeContext::decls instead of scattered around the parser
 - we can remove all the scattered noteClosedVars/Args calls in the emitter
 - we can remove the FlagHeavyweights/SetFunctionKinds analysis in SemanticAnalysis.cpp
Comment 1 Luke Wagner [:luke] 2012-08-01 15:38:01 PDT
Created attachment 648115 [details] [diff] [review]
move bindings construction

Trivial patch to move where we default-construct Bindings (needed for the next patch).
Comment 2 Luke Wagner [:luke] 2012-08-01 15:51:28 PDT
Created attachment 648125 [details] [diff] [review]
create bindings after parsing

This is the main patch.  The basic approach is to remove sc->bindings, hide tc->decls, and funnel everything through a small set of new TreeContext member functions.  Doing this led to a lot of simplifications which were, I suspect, introduced because all this code was so gross.  Overall the diffstats are:

 29 files changed, 679 insertions(+), 906 deletions(-)

One extra piece that got forced into this patch was removing our old complicated scheme for marking functions as heavyweight.  This patch does it more directly: when we generate bindings, we just keep track of whether anything was PND_CLOSED.  Doing this allowed removing a bunch of emitter junk concerned with "noting" closed vars as well as the whole SetFunctionKinds function.
Comment 3 Luke Wagner [:luke] 2012-08-01 15:56:53 PDT
Created attachment 648129 [details] [diff] [review]
rm PN_NAMESET

This is a simple cleanup patch that removes PN_NAMESET.  The only non-trivial change is the removal of the "pnu->pn_blockid < bodyid" disjunct in TreeContext::define.  This is possible because the only case where this arose was a crazy corner case made possible by a crazy interaction with generator expressions and the blob of code removed from LeaveFunction.  With the blob gone, it can't happen.
Comment 4 Luke Wagner [:luke] 2012-08-01 17:00:13 PDT
Created attachment 648147 [details] [diff] [review]
rm PN_NAMESET

Oops, the last version was a bit overzealous with the rippage.
Comment 5 Luke Wagner [:luke] 2012-08-01 17:04:33 PDT
Created attachment 648151 [details] [diff] [review]
rm PN_NAMESET and dead uses

Arg, forgot to qref.
Comment 6 Luke Wagner [:luke] 2012-08-02 14:56:12 PDT
Created attachment 648502 [details] [diff] [review]
combined patch for fuzzing (applies to cset 84ae895140f3)

Sorry for troubling you guys again so soon, but this is hopefully my last patch to the frontend for a while.  It's also a bigun' as it rather significantly changes how we create all declarations.  Fuzzing would be greatly appreciated!
Comment 7 Luke Wagner [:luke] 2012-08-02 14:56:31 PDT
(Green on try.)
Comment 8 Jason Orendorff [:jorendorff] 2012-08-02 15:00:32 PDT
Comment on attachment 648115 [details] [diff] [review]
move bindings construction

stealing this trivial one
Comment 9 Gary Kwong [:gkw] [:nth10sd] 2012-08-02 17:40:56 PDT
Comment on attachment 648502 [details] [diff] [review]
combined patch for fuzzing (applies to cset 84ae895140f3)

Looks good after an hour or two of fuzzing.
Comment 10 Luke Wagner [:luke] 2012-08-02 17:44:35 PDT
inconceivable
Comment 11 Christian Holler (:decoder) 2012-08-02 18:16:51 PDT
This test:

with({x: (x -= 0)}) const [ b ];

asserts with:

Assertion failure: pn->isConst(), at frontend/Parser.cpp:2084
Comment 12 Luke Wagner [:luke] 2012-08-02 22:48:16 PDT
Created attachment 648619 [details] [diff] [review]
create bindings after parsing

Oops, I removed a statement in BindDestructuringVar that I thought was redundant but in fact it was not due to our rather awkward handling of 'with'.
Comment 13 Christian Holler (:decoder) 2012-08-03 04:23:38 PDT
Comment on attachment 648502 [details] [diff] [review]
combined patch for fuzzing (applies to cset 84ae895140f3)

No further asserts or crashes found :)
Comment 14 Luke Wagner [:luke] 2012-08-03 09:06:36 PDT
Thanks!
Comment 15 Christian Holler (:decoder) 2012-08-03 13:21:46 PDT
Hold this for another moment please, I think I got another regression.
Comment 16 Christian Holler (:decoder) 2012-08-06 03:55:22 PDT
(In reply to Christian Holler (:decoder) from comment #15)
> Hold this for another moment please, I think I got another regression.

We sorted this out on IRC, an AutoKeepAtoms wasn't in the right scope.
Comment 17 Christian Holler (:decoder) 2012-08-06 03:55:51 PDT
Another regression:

function test() {
  arguments;
  let (arguments);
}

Assertion failure: !decls_.lookupFirst(name), at js/src/frontend/TreeContext.cpp:44
Comment 18 Luke Wagner [:luke] 2012-08-06 09:13:45 PDT
Ah, great find!  The assertion was correctly pointing out a SyntaxError that wasn't aborting compilation fast enough.
Comment 19 Luke Wagner [:luke] 2012-08-06 09:15:07 PDT
Created attachment 649291 [details] [diff] [review]
create bindings after parsing
Comment 20 Nicholas Nethercote [:njn] 2012-08-07 18:24:07 PDT
Comment on attachment 649291 [details] [diff] [review]
create bindings after parsing

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

::: js/src/jsapi.h
@@ +1065,1 @@
>          HASHABLEVALUE=-25

Should HASHABLEVALUE be changed to -26?

::: js/src/jsscope.h
@@ +264,5 @@
>          WATCHED            =  0x400,
>          ITERATED_SINGLETON =  0x800,
>          NEW_TYPE_UNKNOWN   = 0x1000,
>          UNCACHEABLE_PROTO  = 0x2000,
> +        CLOSED_VAR         = 0x4000,  /* This bit will be removed in bug 767012 */

AFAICT that's the wrong bug number -- bug 767012 is nothing to do with the JS engine.
Comment 21 Luke Wagner [:luke] 2012-08-07 22:55:12 PDT
(In reply to Nicholas Nethercote [:njn] from comment #20)
> Should HASHABLEVALUE be changed to -26?

Oops, yes.

> AFAICT that's the wrong bug number -- bug 767012 is nothing to do with the
> JS engine.

Oops, yes, off by one again: bug 767013.
Comment 22 Luke Wagner [:luke] 2012-08-08 11:47:35 PDT
Created attachment 650225 [details] [diff] [review]
create bindings after parsing

Actually add NAMEVECTOR to AutoGCRooter::trace...
Comment 23 Eddy Bruel [:ejpbruel] 2012-08-14 07:56:26 PDT
Comment on attachment 648151 [details] [diff] [review]
rm PN_NAMESET and dead uses

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

This looks ok.
Comment 24 Eddy Bruel [:ejpbruel] 2012-08-14 07:58:11 PDT
Comment on attachment 650225 [details] [diff] [review]
create bindings after parsing

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

I can't find anything wrong with this patch. Since the patch is so large, that worries me a bit. I requested jorendorff to do an additional review, just to make sure.
Comment 25 Jason Orendorff [:jorendorff] 2012-08-15 16:19:10 PDT
Comment on attachment 650225 [details] [diff] [review]
create bindings after parsing

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

I didn't have enough time to read this closely enough to understand it, but I'm not willing to hold it up any longer. All the parts I looked at closely enough to comprehend seem OK. The main thing that I looked at but couldn't figure out on first glance was the changes to XDR.

I get the impression there was an opportunity here, since all this is so generally poorly understood by the people who are supposed to be reviewers, to patch (incrementally) and teach at the same time.

But this deletes 200 lines of code and I don't see anything wrong.

I am guessing that ejpbruel's r+ means he feels he understands what each line of the patch is trying to do in isolation; and he's worried about bugs of omission and such. If that's correct, by all means let's land it.

::: js/src/frontend/ParseNode.h
@@ +223,5 @@
>   *                                   PNK_STATEMENTLIST node for function body
>   *                                     statements,
>   *                                   PNK_RETURN for expression closure, or
>   *                                   PNK_SEQ for expression closure with
>   *                                     destructured formal parameters

This should cover generator expressions too.

::: js/src/frontend/TreeContext.h
@@ +297,5 @@
> +    void prepareToAddDuplicateArg(Definition *prevDecl);
> +
> +    /* See the sad story in MakeDefIntoUse. */
> +    void updateDecl(JSAtom *atom, ParseNode *newDecl);
> + 

Nit: space on blank line

@@ +304,5 @@
> +     * function's "bindings". Bindings are a data-structure, ultimately stored
> +     * in the compiled JSScript, that serve three purposes:
> +     *  - After parsing, the TreeContext is destroyed and 'decls' along with
> +     *    it. Mostly, the emitter just uses the binding information stored in
> +     *    the use/def nodes, but the emitter occaisionally needs 'bindings' for

Nit: Spelling "occasionally".

::: js/src/vm/Debugger.cpp
@@ +3707,5 @@
>              BindingVector names(cx);
>              if (!GetOrderedBindings(cx, fun->script()->bindings, &names))
>                  return false;
> +            for (size_t i = 0; i < fun->nargs; i++)
> +                result->setDenseArrayElement(i, StringValue(names[i].name));

Instead of changing the behavior and the test, why not just detect when the name is empty and substitute undefined?
Comment 26 Jason Orendorff [:jorendorff] 2012-08-15 16:19:55 PDT
Luke: if you *want* to walk me through this patch and show me what's going on here, feel free -- we can do it before or after it lands.
Comment 27 Luke Wagner [:luke] 2012-08-15 16:57:33 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #25)
Thanks!

> Instead of changing the behavior and the test, why not just detect when the
> name is empty and substitute undefined?

Sounds good.

(In reply to Jason Orendorff [:jorendorff] from comment #26)

I'm happy to discuss how it works if anyone has any questions.

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