Closed Bug 1494930 Opened 6 years ago Closed 6 years ago

[BinAST] Correct differences in emitted bytecode

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: efaust, Assigned: efaust)

Details

Attachments

(4 files)

Various corrections/improvements to the bytecode we'll generate from the BinAST parser.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #9012837 - Flags: review?(arai.unmht)
Attachment #9012838 - Flags: review?(arai.unmht)
Attachment #9012839 - Flags: review?(arai.unmht)
Attachment #9012840 - Flags: review?(arai.unmht)
Comment on attachment 9012837 [details] [diff] [review]
[Part 1] Various fixes

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

::: js/src/frontend/BinSource.cpp
@@ +221,4 @@
>          // TODO (efaust): This capture will have to come from encoder side for arrow functions.
>      }
>  
> +    HandlePropertyName arguments = cx_->names().arguments;

can you add a comment which refers PerHandlerParser<ParseHandler>::declareFunctionArgumentsObject ?
there are some difference between the implementation here and declareFunctionArgumentsObject.
it would be nice to add comment explains why, or maybe just add TODO

@@ +221,5 @@
>          // TODO (efaust): This capture will have to come from encoder side for arrow functions.
>      }
>  
> +    HandlePropertyName arguments = cx_->names().arguments;
> +    if (hasUsedName(arguments)) {

can you add funbox->bindingsAccessedDynamically() to the condition?
https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/js/src/frontend/Parser.cpp#2958
https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/js/src/frontend/Parser.cpp#2609

@@ +229,5 @@
> +            BINJS_TRY(funScope.addDeclaredName(parseContext_, p, arguments, DeclarationKind::Var,
> +                                               DeclaredNameInfo::npos));
> +        }
> +        funbox->declaredArguments = true;
> +        funbox->usesArguments = true;

in Parser, above 2 are done only when p is falsy.
it should be better doing in the same way, or adding the comment that explains why it's different.

@@ +232,5 @@
> +        funbox->declaredArguments = true;
> +        funbox->usesArguments = true;
> +
> +        funbox->setArgumentsHasLocalBinding();
> +        if (funbox->bindingsAccessedDynamically())

braces

to be clear, it's not necessary to fix it in this patch, if it may cause conflict.
it's fine to fix all of them later.
you can use jandem's script in bug 1488698 to automatically fix them.

@@ +483,4 @@
>          }
>          prefix->setKind(body->getKind());
>          prefix->setOp(body->getOp());
> +        if (body->hasTopLevelFunctionDeclarations())

and here
Attachment #9012837 - Flags: review?(arai.unmht) → review+
Attachment #9012838 - Flags: review?(arai.unmht) → review+
Comment on attachment 9012839 [details] [diff] [review]
[Part 3] Don't generate both deffun and defvar.

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

::: js/src/frontend/BinSource.cpp
@@ +166,4 @@
>          atom = name->name();
>      }
>  
> +    if (parseContext_ && syntax == FunctionSyntaxKind::Statement) {

I don't think parseContext_ can be nullptr here.
Attachment #9012839 - Flags: review?(arai.unmht) → review+
Comment on attachment 9012840 [details] [diff] [review]
[Part 4] Respect strictness passed in directives

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

::: js/src/frontend/SharedContext.h
@@ +229,5 @@
>          localStrict = strict;
>          return retVal;
>      }
> +    void forceStrictScript() {
> +        strictScript = true;

can you use this method in Parser?
https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/js/src/frontend/Parser.cpp#4608
Attachment #9012840 - Flags: review?(arai.unmht) → review+
Pushed by efaustbmo@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/864b718cd797
Part 1: Various BinAST parser correctness fixes. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb50ca3776e
Part 2: Correctly generate JSOP_FUNCALL and JSOP_FUNAPPLY. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8544285b4145
Part 3: Don't generate both JSOP_DEFVAR and JSOP_DEFFUN for toplevel functions. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/275f7949148b
Part 4: Respect strictness directives. (r=arai)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63ea63571271
Backed out 10 changesets (bug 1494930, bug 1459067, bug 1459555) for build bustages on JSScript.cpp. CLOSED TREE
Pushed by efaustbmo@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90003e4c4f3a
Part 1: Various BinAST parser correctness fixes. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc8180f3621
Part 2: Correctly generate JSOP_FUNCALL and JSOP_FUNAPPLY. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/afa82a9888b4
Part 3: Don't generate both JSOP_DEFVAR and JSOP_DEFFUN for toplevel functions. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8820b0b524e0
Part 4: Respect strictness directives. (r=arai)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: