Closed Bug 775323 Opened 10 years ago Closed 9 years ago

build Bindings after, not during, parsing

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: [js:t])

Attachments

(4 files, 5 obsolete files)

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
Whiteboard: [js:t]
Depends on: 775684
Depends on: 775734
Depends on: 772328
Depends on: 778919
Trivial patch to move where we default-construct Bindings (needed for the next patch).
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #648115 - Flags: review?(ejpbruel)
Attached patch create bindings after parsing (obsolete) — Splinter Review
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.
Attached patch rm PN_NAMESET (obsolete) — Splinter Review
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.
Attachment #648129 - Flags: review?(ejpbruel)
Attached patch rm PN_NAMESET (obsolete) — Splinter Review
Oops, the last version was a bit overzealous with the rippage.
Attachment #648129 - Attachment is obsolete: true
Attachment #648129 - Flags: review?(ejpbruel)
Attachment #648147 - Flags: review?(ejpbruel)
Attachment #648125 - Flags: review?(ejpbruel)
Arg, forgot to qref.
Attachment #648147 - Attachment is obsolete: true
Attachment #648147 - Flags: review?(ejpbruel)
Attachment #648151 - Flags: review?(ejpbruel)
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!
Attachment #648502 - Flags: feedback?(gary)
Attachment #648502 - Flags: feedback?(choller)
(Green on try.)
Comment on attachment 648115 [details] [diff] [review]
move bindings construction

stealing this trivial one
Attachment #648115 - Flags: review?(ejpbruel) → review+
Comment on attachment 648502 [details] [diff] [review]
combined patch for fuzzing (applies to cset 84ae895140f3)

Looks good after an hour or two of fuzzing.
Attachment #648502 - Flags: feedback?(gary) → feedback+
inconceivable
This test:

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

asserts with:

Assertion failure: pn->isConst(), at frontend/Parser.cpp:2084
Attached patch create bindings after parsing (obsolete) — Splinter Review
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'.
Attachment #648125 - Attachment is obsolete: true
Attachment #648125 - Flags: review?(ejpbruel)
Attachment #648619 - Flags: review?(ejpbruel)
Comment on attachment 648502 [details] [diff] [review]
combined patch for fuzzing (applies to cset 84ae895140f3)

No further asserts or crashes found :)
Attachment #648502 - Flags: feedback?(choller) → feedback+
Thanks!
Hold this for another moment please, I think I got another regression.
(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.
Another regression:

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

Assertion failure: !decls_.lookupFirst(name), at js/src/frontend/TreeContext.cpp:44
Ah, great find!  The assertion was correctly pointing out a SyntaxError that wasn't aborting compilation fast enough.
Attached patch create bindings after parsing (obsolete) — Splinter Review
Attachment #648619 - Attachment is obsolete: true
Attachment #648619 - Flags: review?(ejpbruel)
Attachment #649291 - Flags: review?(ejpbruel)
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.
(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.
Actually add NAMEVECTOR to AutoGCRooter::trace...
Attachment #649291 - Attachment is obsolete: true
Attachment #649291 - Flags: review?(ejpbruel)
Attachment #650225 - Flags: review?(ejpbruel)
Comment on attachment 648151 [details] [diff] [review]
rm PN_NAMESET and dead uses

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

This looks ok.
Attachment #648151 - Flags: review?(ejpbruel) → review+
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.
Attachment #650225 - Flags: review?(jorendorff)
Attachment #650225 - Flags: review?(ejpbruel)
Attachment #650225 - Flags: review+
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?
Attachment #650225 - Flags: review?(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.
(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.
Depends on: 784550
You need to log in before you can comment on or make changes to this bug.