assert some invariants in BindNameToSlot

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla16
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 639215 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

The patch adds a couple asserts/comments that (I think) hold and are needed for bug 753158.  Some fuzzing would be appreciated and increase confidence.
Attachment #639215 - Flags: review?(jorendorff)
Attachment #639215 - Flags: feedback?(gary)
Comment on attachment 639215 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

decoder might also be able to help.
Attachment #639215 - Flags: feedback?(choller)
(Assignee)

Comment 2

5 years ago
woohoo, so far green on try.
I'm on this now :)
Comment on attachment 639215 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

Green for me after a night of overnight fuzzing on Mac 10.7 as well.
Attachment #639215 - Flags: feedback?(gary) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 639507 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

Another key invariant (different static level implies isClosed) and a few more syntactic cleanups.
Attachment #639215 - Attachment is obsolete: true
Attachment #639215 - Flags: review?(jorendorff)
Attachment #639215 - Flags: feedback?(choller)
Attachment #639507 - Flags: review?(jorendorff)
Attachment #639507 - Flags: feedback?(choller)
(Assignee)

Comment 6

5 years ago
Created attachment 639568 [details] [diff] [review]
patch (applies to cset 07b1a5999430)

... and empowered by these assertions, I can see that this atomIndex-abusing abomination is dead.
Attachment #639507 - Attachment is obsolete: true
Attachment #639507 - Flags: review?(jorendorff)
Attachment #639507 - Flags: feedback?(choller)
Attachment #639568 - Flags: review?(jorendorff)
Attachment #639568 - Flags: feedback?(choller)
Not going to let you get away so easily ^^ (run with -n -m):


const cases = [{ expr: "((function() arguments) for (x in []))" }];
function expectError1(err, ctx, msg) {}
function expectError(expr, call, wrapCtx, expect, msg) {
    expectError1(eval(wrapCtx(call)), exps.call, 'call argument in ' + msg);
}
function atTop(str) { return str; }
  for (let i = 0, len = cases.length; i < len; i++) {
    let {expr, top, fun, gen, desc} = cases[i];
    let call = (expr[0] === "(") ? ("print" + expr) : null;
      expectError(null, call, atTop);
}


Assertion failure: dn->isClosed(), at js/src/frontend/BytecodeEmitter.cpp:1274
(Assignee)

Comment 8

5 years ago
Which boils down to: ((function() arguments) for (x in []));
Mmm, 'arguments' and generator expressions, what could go wrong?
(Assignee)

Comment 9

5 years ago
Created attachment 639871 [details] [diff] [review]
fix genexpr transplanting

Ah ha, this is exactly what I was hoping to flush out.  There is a subtle interaction whereby implicit arguments gets bound at one static level, and then later, when we realize we are in a generator expression, we try to fix up all the parse nodes.  However, since there was no explicit definition of the arguments, CompExprTransplanter::transplant doesn't update the static level of the implicit arguments.  This the fix is pretty simple, but regrettably adds a PND flag.
Attachment #639871 - Flags: review?(jorendorff)
(Assignee)

Comment 10

5 years ago
Created attachment 639872 [details] [diff] [review]
combined patch for fuzzing (applies to cset 007003bb82c9)
Attachment #639872 - Flags: feedback?(choller)
(Assignee)

Updated

5 years ago
Attachment #639568 - Flags: feedback?(choller)
Comment on attachment 639872 [details] [diff] [review]
combined patch for fuzzing (applies to cset 007003bb82c9)

No further bugs found so far, feedback+.
Attachment #639872 - Flags: feedback?(choller) → feedback+
Whiteboard: [js:t]
(Assignee)

Updated

5 years ago
Attachment #639568 - Flags: review?(jorendorff) → review?(dvander)
(Assignee)

Updated

5 years ago
Attachment #639871 - Flags: review?(jorendorff) → review?(dvander)
Attachment #639568 - Flags: review?(dvander) → review+
Comment on attachment 639871 [details] [diff] [review]
fix genexpr transplanting

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

::: js/src/frontend/Parser.cpp
@@ +5102,5 @@
> +                    if (genexp && !visitedImplicitArguments.has(dn)) {
> +                        if (!BumpStaticLevel(dn, tc))
> +                            return false;
> +                        AdjustBlockId(dn, adjust, tc);
> +                        if (!visitedImplicitArguments.put(dn))

Out of curiosity, would it make sense to just strip the PND_IMPLICITARGUMENTS flag so you don't visit it again?
Attachment #639871 - Flags: review?(dvander) → review+
(Assignee)

Comment 13

5 years ago
(In reply to David Anderson [:dvander] from comment #12)
Ah, yes, that would work.  The reason I'd prefer the clunkier HashSet is that if we unset the bit, it would nuance the official meaning of PND_IMPLICITARGUMENTS from "is an implicitly-declared arguments" to "is a bit set that CompExprTransplanter uses to x y z..." which, in some sense, let's CompExprTransplanter spread it's grossness.
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/41d5c8529748
https://hg.mozilla.org/integration/mozilla-inbound/rev/0253f34f6bc2
Target Milestone: --- → mozilla16
Unfortunately bug 772303 (https://hg.mozilla.org/integration/mozilla-inbound/rev/fad7d06d7dd5) had to be backed out for winxp pgo-only jsreftest failures.

This bug (amongst others) either caused unresolvable (for someone who doesn't work on the JS engine at least) conflicts with the backout of fad7d06d7dd5, or else bugzilla dependencies indicated that one of it's dependants had now been backed out. 

Since there was no one in #developers that could resolve the conflicts, unfortunately this bug has been backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0be4b70b814
Target Milestone: mozilla16 → ---
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/15409e3b3bdc
(Assignee)

Comment 17

5 years ago
oops, and:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b790407d394f
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/15409e3b3bdc
https://hg.mozilla.org/mozilla-central/rev/b790407d394f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 773927
You need to log in before you can comment on or make changes to this bug.