600884 - JM: "Assertion failure: LookupLoop(traceMonitor, tree->ip, tree->globalObj, tree->globalShape, tree->argc) == tree->first,"

Status: RESOLVED DUPLICATE of bug 600889

(Core :: JavaScript Engine, defect)

Description

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

function z(zz) {
    if (zz) this.e = null
}
for each(let a in [Number(), 0, 0, 0, 0, 0, new String(), new String(), 0, 0]) {
    try {
        let y = new z(a)
        gc()
    } catch (e) {}
}

asserts js debug shell on TM changeset 98c134cf59ef with -m and -j at Assertion failure: LookupLoop(traceMonitor, tree->ip, tree->globalObj, tree->globalShape, tree->argc) == tree->first,

Setting s-s because this involves gc.

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54627:81881086131a
user:        Brian Hackett
date:        Wed Sep 29 06:27:34 2010 -0700
summary:     Improved JM call path, bug 587707. r=lw,dvander

Comment 2

[Brian Hackett [Laid off!]]

I don't think this was caused by 81881086131a.

The assertion is TM looking for a tree fragment which was disconnected by TraceMonitor::sweep because it contains a dead GC thing (the shape made by 'this.e = null').  It looks like two traces get created here.  The first in a 'new String()' iteration, where the 'if (zz)' test passes, branch taken and shape used.  That falls off trace shortly when the branch isn't taken, and a second trace is later recorded, which has the first trace as its 'tree->first'.  During recording of the second trace the first trace is GC'ed, because the shape it contains is no longer reachable, and we assert.

81881086131a doesn't touch any of this.  If I use an earlier revision then the shape isn't collected, presumably because it is still reachable from the C stack via the conservative collector (also, mildly perturbing the testcase in unrelated ways makes this assert go away).  The shape is garbage though, and should be collected.

I don't know what the right fix is.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> I don't think this was caused by 81881086131a.
> 
> The assertion is TM looking for a tree fragment which was disconnected by
> TraceMonitor::sweep because it contains a dead GC thing (the shape made by
> 'this.e = null').  It looks like two traces get created here.  The first in a
> 'new String()' iteration, where the 'if (zz)' test passes, branch taken and
> shape used.  That falls off trace shortly when the branch isn't taken, and a
> second trace is later recorded, which has the first trace as its 'tree->first'.
>  During recording of the second trace the first trace is GC'ed, because the
> shape it contains is no longer reachable, and we assert.

At some earlier point, shapes (then sprops) and other GC'ed immediates were kept in trace-JITted code and marked, making them strong refs. Now it seems they are treated as weak refs and tested in TM::sweep to see whether they're not marked.

> 81881086131a doesn't touch any of this.  If I use an earlier revision then the
> shape isn't collected, presumably because it is still reachable from the C
> stack via the conservative collector (also, mildly perturbing the testcase in
> unrelated ways makes this assert go away).  The shape is garbage though, and
> should be collected.

This can't be the full story, as Shapes are not GC-things, so not scanned via conservatively interpreted stack word maybe-refs.

> I don't know what the right fix is.

I don't see the assertbotch on tm tip, with -j or -m -j. Was it just a bogus assertion?

/be

Comment 4

[Brian Hackett [Laid off!]]

(In reply to comment #3)
> This can't be the full story, as Shapes are not GC-things, so not scanned via
> conservatively interpreted stack word maybe-refs.

Yeah, the shape can only get marked via one of the 'new z(a)' objects allocated in either of the 'new String()' iterations.  If either of those objects still has a lingering reference on the C stack, the shape will not be collected.

> I don't see the assertbotch on tm tip, with -j or -m -j. Was it just a bogus
> assertion?

I get the failure on tip (rev 2824ef10a50f) on OS X 10.6 under x86.  Since the conservative GC is involved this bug is pretty fragile.

Comment 5

[Brendan Eich [:brendan]]

Any ill effects on optimized builds, under valgrind, etc.?

I'm still wondering if this isn't a bogus assertion.

/be

Comment 6

[Brian Hackett [Laid off!]]

(In reply to comment #5)
> Any ill effects on optimized builds, under valgrind, etc.?
> 
> I'm still wondering if this isn't a bogus assertion.
> 
> /be

It might be bogus, I don't know the TM code well enough to tell.  At the assert, tree->first points to the fragment which referenced the shape, and which has now gone through TrashTree.  TrashTree doesn't seem to free the fragment though (how are TreeFragments destroyed?), and I don't know if it will continue to work OK.  Optimized build doesn't crash on me.

Comment 7

[Brendan Eich [:brendan]]

Need some TM trace-trees expert to weigh in. Andreas, dvander? IIRC Igor was patching around related code.

/be

Comment 8

[Andreas Gal :gal]

Yeah, this looks related to Igor's fragment sweeping changes.

Comment 9

[Johnny Stenback (:jst)]

Igor, any thoughts here?

Comment 10

[Johnny Stenback (:jst)]

Igor, ping?

Comment 11

[Igor Bukanov]

I believe this is related to the bug 600889. Gal, could you comment there on some issues regarding your suggestion.

Comment 12

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

Variations assert at Assertion failure: tree->first == LookupLoop(traceMonitor, ip, tree->globalObj, tree->globalShape, tree->argc), but usually get reduced to the assert in this bug.

Comment 13

[David Mandelin [:dmandelin]]

(In reply to comment #11)
> I believe this is related to the bug 600889. Gal, could you comment there on
> some issues regarding your suggestion.

Indeed, this bug WFM on tip and the first rev where it works is:

changeset:   58587:5177ee4c10d6
user:        Igor Bukanov <[e-mail]>
date:        Mon Oct 04 23:08:11 2010 +0200
summary:     Bug 600889 - TraceMonitor::sweep() should abort recording if it trashes the recorder tree. r=gal

Comment 14

[Christian Holler (:decoder)]

A testcase for this bug was already added in the original bug (bug 600889).