775323 - build Bindings after, not during, parsing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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]]

Attachment: move bindings construction

Trivial patch to move where we default-construct Bindings (needed for the next patch).

Comment 2

[Luke Wagner [:luke]]

Attachment: 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]]

Attachment: 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]]

Attachment: rm PN_NAMESET

Oops, the last version was a bit overzealous with the rippage.

Comment 5

[Luke Wagner [:luke]]

Attachment: rm PN_NAMESET and dead uses

Arg, forgot to qref.

Comment 6

[Luke Wagner [:luke]]

Attachment: 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]]

(Green on try.)

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 648115 [details] [diff] [review]
move bindings construction

stealing this trivial one

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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]]

inconceivable

Comment 11

[Christian Holler (:decoder)]

This test:

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

asserts with:

Assertion failure: pn->isConst(), at frontend/Parser.cpp:2084

Comment 12

[Luke Wagner [:luke]]

Attachment: 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)]

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]]

Thanks!

Comment 15

[Christian Holler (:decoder)]

Hold this for another moment please, I think I got another regression.

Comment 16

[Christian Holler (:decoder)]

(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)]

Another regression:

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

Assertion failure: !decls_.lookupFirst(name), at js/src/frontend/TreeContext.cpp:44

Comment 18

[Luke Wagner [:luke]]

Ah, great find!  The assertion was correctly pointing out a SyntaxError that wasn't aborting compilation fast enough.

Comment 19

[Luke Wagner [:luke]]

Attachment: create bindings after parsing

Comment 20

[Nicholas Nethercote [inactive]]

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]]

(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]]

Attachment: create bindings after parsing

Actually add NAMEVECTOR to AutoGCRooter::trace...

Comment 23

[Eddy Bruel [:ejpbruel]]

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]]

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]]

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]]

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]]

(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.

Comment 28

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/858ab0d138a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/48cfc16cac71
https://hg.mozilla.org/integration/mozilla-inbound/rev/401453e782ab

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/858ab0d138a0
https://hg.mozilla.org/mozilla-central/rev/48cfc16cac71
https://hg.mozilla.org/mozilla-central/rev/401453e782ab