Closed Bug 553778 Opened 10 years ago Closed 9 years ago

"Assertion failure: cg->fun->u.i.skipmin <= skip, at ../jsemit.cpp"

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- beta8+
status1.9.2 --- wanted
status1.9.1 --- wanted

People

(Reporter: gkw, Assigned: jimb)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 file, 1 obsolete file)

(function () {
    var a = function () {
        (function () {
            a, x::e
        }[0])
        for (var [x] = [];;) {}
    }
}[0])


asserts js debug shell on TM tip without -j at Assertion failure: cg->fun->u.i.skipmin <= skip, at ../jsemit.cpp:2224

autoBisect shows this is probably related to bug 494235:

The first bad revision is:
changeset:   28920:c074f0f0ad2d
user:        Brendan Eich
date:        Thu Jun 04 18:58:47 2009 -0700
summary:     Bug 494235: wrap escaping optimized closures for the debugger API (r=igor/mrbkap).
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Gary: why does this bug potentially block a branch release?
marking affected branches for now, re-nom for blocking if there's compelling reasons that it fits the stable branch criteria
blocking1.9.2: ? → ---
(In reply to comment #1)
> Gary: why does this bug potentially block a branch release?

I have no idea if this assertion is bad, also, it seems to have regressed since June 2009. I guess I should have nominated first for 1.9.3.
<nth12sd> brendan: is bug 553778 worth taking a look?
<brendan> nth12sd: jorendorff can fix it i bet
E4X is innocent. This asserts the same way:

function f() {
    function g() {
        function h() {
            g; x;
        }
        var [x] = [];
    }
}

In h, skipMin is 2, reflecting that at the time h was parsed, g was 2 frames up (in f) and x was apparently a global.  If I change the declaration on the 6th line to just `var x = [];` then it doesn't assert.
blocking2.0: ? → beta1+
blocking2.0: beta1+ → beta2+
blocking2.0: beta2+ → betaN+
Can we get an owner here, please?
Assignee: general → jimb
blocking2.0: betaN+ → beta8+
Assignee: jimb → jimb
Looking at this...
The problem here appears to be that destructuring var declarations don't correctly set the cookie:

'var x = []' produces a definition node for x whose upvar cookie is 2:1, whereas 'var [x] = []' produces a definition node for x whose upvar cookie is FREE_VALUE (completely incorrect).
FindFunArgs walks the tree of functions and scans each function's upvars (as recorded in the TOK_UPVARS node, if present) to compute the function's skipmin.

In the test case, when we see the first reference to x, we create a placeholder definition node for that to refer to. The placeholder goes in h's upvar table. When we see the declaration of x, we create a new definition node for that, steal the original definition node's uses, and update their lexdef links, as per standard practice. The original definition node is left with an empty use list; it does not refer to our new definition node at all, as there are cases where such placeholder nodes still have uses.

However, we leave the original placeholder definition in h's upvars table. Since we've done nothing to associate this with the new definition node, FindFunArgs doesn't recognize that its upvar has found a definition, and thus computes its skipmin value incorrectly.
So, I think I've gotten my head around what's going on in jsparse.cpp:Define, which we call to introduce a new JSDefinition node for some identifier. What's subtle about this function is not just that we may already have a placeholder definition node for our identifier, with a chain of use nodes, but that our new definition may steal none, some portion, or all of that use list, as its own.

Here, the call to Define for the 'let' will steal none of the uses from the var node's use list:

function f() { var x = 3; x; { let x = 4; } return x; }

Here, the call to Define for the 'let' will steal one use off the var node's use list: when we see the 'let', we realize that the second reference to x actually doesn't belong to the 'var' at all:

function f() { var x = 3; x; { x = 4; let x; } return x; }

Here, the call to Define will steal all the uses from the placeholder:

function f() { function g() { x; } let [x] = [1]; }

The nice solution to the problem described in comment 9 would be to simply transform the placeholder node, listed in the inner functions' TOK_UPVARS nodes, into a use of the capturing definition; then the 'resolve' call in FindFunArgs will find the right definition, and compute the function's skipmin correctly.

But that would not be correct if those placeholder node can have *some* uses that the new definition should capture, and others that it must not. Can the placeholder merging done in LeaveFunction construct such use lists?

Here is an example to try in the morning:

function f() { x; { x; function g() { x; } let x; } }

I believe that 1) the first and second uses of x will be on the same placeholder's use list, 2) the lexdep from g will be merged with that placeholder, but then 3) the 'let' will steal the second and third references, but not the first. Thus, the placeholder in g's TOK_UPVAR node cannot be transformed into a use, because that would incorrectly turn the first use of x into a use of the 'let'-bound x.
I think that process of "turn this def into a use of this new def" should be abstracted out into a function (even though we already have a MakeUseIntoDef, which does something else). But I'd appreciate a review just to see if I'm thinking about this in a reasonable way.
(In reply to comment #12)
> I think that process of "turn this def into a use of this new def" should be
> abstracted out into a function (even though we already have a MakeUseIntoDef,

You mean MakeDefIntoUse?

> which does something else

MakeDefIntoUse does what it said, with a complication but it's not doing something else, AFAIK. What do you mean?

> But I'd appreciate a review just to see if I'm thinking about this in a
> reasonable way.

Looking now.

/be
(In reply to comment #10)
> So, I think I've gotten my head around what's going on in jsparse.cpp:Define,
> which we call to introduce a new JSDefinition node for some identifier. What's
> subtle about this function is not just that we may already have a placeholder
> definition node for our identifier, with a chain of use nodes, but that our new
> definition may steal none, some portion, or all of that use list, as its own.

The joys of hoisting.

> Here, the call to Define for the 'let' will steal one use off the var node's
> use list: when we see the 'let', we realize that the second reference to x
> actually doesn't belong to the 'var' at all:
> 
> function f() { var x = 3; x; { x = 4; let x; } return x; }

BTW, we resolved without yet spec'ing it even informally to change let in Harmony to be like a declaration in C++, or like let* if you will, and not hoist (yay!). I convinced everyone this is what people expect and not var-like hoisting. So we should have a bug to implement this, and get on with it -- and break JS1.[78]* compatibility too.

> Here, the call to Define will steal all the uses from the placeholder:
> 
> function f() { function g() { x; } let [x] = [1]; }
> 
> The nice solution to the problem described in comment 9 would be to simply
> transform the placeholder node, listed in the inner functions' TOK_UPVARS
> nodes, into a use of the capturing definition; then the 'resolve' call in
> FindFunArgs will find the right definition, and compute the function's skipmin
> correctly.
> 
> But that would not be correct if those placeholder node can have *some* uses
> that the new definition should capture, and others that it must not.

Right, that is why the code instead steals uses. But we shouldn't leave an empty placeholder in an exited function's upvars -- this is the bug.

> Can the
> placeholder merging done in LeaveFunction construct such use lists?
> 
> Here is an example to try in the morning:
> 
> function f() { x; { x; function g() { x; } let x; } }
> 
> I believe that 1) the first and second uses of x will be on the same
> placeholder's use list, 2) the lexdep from g will be merged with that
> placeholder, but then 3) the 'let' will steal the second and third references,
> but not the first.

Right.

> Thus, the placeholder in g's TOK_UPVAR node cannot be
> transformed into a use, because that would incorrectly turn the first use of x
> into a use of the 'let'-bound x.

That's right, but if an exited function has an upvar in its TOK_UPVARS set that is a useless placeholder (whew!), then we hit this bug. One fix would be to cope in LeaveFunction. Looks like you attacked the problem in Define. More in a bit, on the patch.

/be
Comment on attachment 487722 [details] [diff] [review]
Don't orphan placeholder definition nodes when a real definition is found.

>Bug 553778: Don't orphan placeholder definition nodes when a real definition is found.
>
>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>--- a/js/src/jsparse.cpp
>+++ b/js/src/jsparse.cpp
>@@ -1467,8 +1467,33 @@ Define(JSParseNode *pn, JSAtom *atom, JS
>                 pn->dn_uses = dn->dn_uses;
>                 dn->dn_uses = pnu;
> 
>-                if ((!pnu || pnu->pn_blockid < tc->bodyid) && list != &tc->decls)
>-                    list->rawRemove(tc->parser, ale, hep);
>+                /*
>+                 * If we followed the use list all the way back to tc's outermost block,
>+                 * then we must remove it from its list: either we have satisfied all
>+                 * references to that identifier, so it is no longer a lexdep, or
>+                 * we are about to replace the decls entry with our new definition.
>+                 */
>+                if (!pnu || pnu->pn_blockid < tc->bodyid) {
>+                    /* We needn't bother deleting a tc->decls entry; we'll overwrite it below. */
>+                    if (list != &tc->decls) {
>+                        list->rawRemove(tc->parser, ale, hep);

Is the if(||)-if instead of if((||)&&) better? It's more indented :-|.

>+                        /*
>+                         * If we've taken all the definitions from dn, then we must

"all the uses from dn"

>+                         * transform it into a use of our new definition pn: dn may appear
>+                         * in inner functions' TOK_UPVARS tables, and those should resolve
>+                         * to our definition.
>+                         */
>+                        if (!pnu) {
>+                            dn->pn_link = pn->dn_uses;
>+                            pn->dn_uses = dn;
>+                            dn->pn_dflags &= ~PND_PLACEHOLDER;
>+                            pn->pn_dflags |= dn->pn_dflags & PND_USE2DEF_FLAGS;
>+                            dn->pn_defn = false;
>+                            dn->pn_used = true;
>+                            dn->pn_lexdef = (JSDefinition *) pn;

You're right that there is a more primitive MakeDefIntoUse to common out here. I still wonder about making the fix in LeaveFunction instead. Would that be even simpler? It would leave a useless placeholder around for a while.

/be
(In reply to comment #15)
> >+                if (!pnu || pnu->pn_blockid < tc->bodyid) {
> >+                    /* We needn't bother deleting a tc->decls entry; we'll overwrite it below. */
> >+                    if (list != &tc->decls) {
> >+                        list->rawRemove(tc->parser, ale, hep);
> 
> Is the if(||)-if instead of if((||)&&) better? It's more indented :-|.

Uh-oh, a ':-|' from Brendan --- that means he's pretty upset!! :)

I could not understand that test until I realized that it was just an optimization --- it's always appropriate to remove the entry, but in the decls case we're going to add it right back. I like it better this way, but I'd be happy enough with a good comment.

> >+                         * If we've taken all the definitions from dn, then we must
> 
> "all the uses from dn"

Gaack.

> You're right that there is a more primitive MakeDefIntoUse to common out here.
> I still wonder about making the fix in LeaveFunction instead. Would that be
> even simpler? It would leave a useless placeholder around for a while.

That could well be --- and seems more "right".  I'll give that a shot tomorrow.

I also have a bunch more test cases to add that helped me understand why the code must be as it is.
(In reply to comment #14)
> BTW, we resolved without yet spec'ing it even informally to change let in
> Harmony to be like a declaration in C++, or like let* if you will, and not
> hoist (yay!). I convinced everyone this is what people expect and not var-like
> hoisting. So we should have a bug to implement this, and get on with it -- and
> break JS1.[78]* compatibility too.

That sounds great to me. The behavior in the example is pretty surprising.

> Right, that is why the code instead steals uses. But we shouldn't leave an
> empty placeholder in an exited function's upvars -- this is the bug.

Yep, as stated in comment 9.
(In reply to comment #14)
> BTW, we resolved without yet spec'ing it even informally to change let in
> Harmony to be like a declaration in C++, or like let* if you will, and not
> hoist (yay!). I convinced everyone this is what people expect and not var-like
> hoisting. So we should have a bug to implement this, and get on with it -- and
> break JS1.[78]* compatibility too.

Filed as bug 609205.
Okay, this is revised to do the deed in LeaveFunction. I think this is much nicer; please let me know what you think.
Attachment #488007 - Flags: review?(brendan)
Attachment #487722 - Attachment is obsolete: true
Attachment #487722 - Flags: review?(brendan)
Comment on attachment 488007 [details] [diff] [review]
Don't orphan placeholder definition nodes when a real definition is found.

Great, nice attention to detail. Commoning the pn_type and pn_op init wins in saved lines of code and less distributed knowledge. The runt-else for the old if (outer_ale) if going away thanks to MakePlaceholder doing lexdep.add, etc., ditto.

/be
Attachment #488007 - Flags: review?(brendan) → review+
Below from the Try server. This seems sufficiently unsimilar to the other intermittent failures in this test (31 assertions?) that I'm going to look into it.

/tests/dom/tests/mochitest/ajax/scriptaculous/test_Scriptaculous.html | testInPlaceEditor - 31 assertions, 2 failures, 0 errors
Scriptaculous failures seemed spurious; landed.
http://hg.mozilla.org/tracemonkey/rev/8c59a5bf187b
Whiteboard: [fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/8c59a5bf187b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-553778.js.
Flags: in-testsuite+
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.