620637 - Assertion failure: result->isImmI(jsint(r))

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following code:

options('tracejit');
var actual = ''
function f() {
    for (var a = 0; a < 3; ++a) {
        (function () {
            for (var b = 0; b < 2; ++b) {
                (function () {
                    for (a = 0, b = 0; b < 15; b++, actual = actual + "7") {
                    }
                })();
            }
        })();
    }
}
f();

causes Assertion failure: result->isImmI(jsint(r)), at jstracer.cpp:8390

verified on mozilla-central b8 and trunk.

Comment 1

[Nicholas Nethercote [inactive]]

This is the culprit:

changeset:   53418:8721b595e7ab
user:        Luke Wagner <[e-mail]>
date:        Mon Aug 09 22:43:33 2010 -0700
summary:     Bug 539144 - Make formal args a jit-time const offset from fp; rm argv/argc/thisv/script/callobj (r=brendan,dvander)

Comment 2

[Luke Wagner [:luke]]

Attachment: test case that asserts before 8721b595e7ab

Comment 3

[Luke Wagner [:luke]]

This bug stems from a faulty assumption about how inner trees can mutate outer trees' upvars that predates bug 539144 (as demonstrated by the attached testcase).  The hole in the reasoning here (http://hg.mozilla.org/tracemonkey/file/0be4f01086ea/js/src/jstracer.cpp#l5257) is that 4 is not strong enough to imply 5 since we do not check scopeChain identity when entering trace.  Rather than fixing this by remembering and checking the scopeChain, I'd rather just remove the whole optimization and just clear everything when calling an inner tree.

This can be done quite simplfy by factoring clearCurrentFrameSlotsFromTracker into a visitor that reuses VisitFrameSlots.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix this bug, expose another

This does what comment 3 says and doesn't assert for the test-case in comment 0 (which iloops).  Unfortunately, this exposes another invalid assumption (which, coincidentally, hits the test case I just posted in attachment 499407 [details]): the elements of importStackSlots that get left as JSVAL_TYPE_UNINITIALIZED by emitTreeCall() are now being observed (and thus, asserting) since the corresponding entries in the tracker are now getting cleared.  The fix is to add an extra bit of logic to emitTreeCall to fill these elements with valid types.  On the bright side, this should allow us kill JSVAL_TYPE_UNINITIALIZED :)

Comment 5

[Luke Wagner [:luke]]

Attachment: better fix

The last patch had a bug in that it only cleared up to 'sp' instead of 'slots + script->nslots'.

Comment 6

[Luke Wagner [:luke]]

Attachment: don't be so pessimistic

So it turns out that attemptTreeCall was already doing the necessary preparation of importTypeMap so, IIUC, all the JSVAL_TYPE_UNINITIALIZED-filling wasn't necessary.

Comment 7

[Luke Wagner [:luke]]

Oops, assume the two test cases above are included (and un-iloop-ed) in those patches.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 499485 [details] [diff] [review]
better fix

>+TraceRecorder::clearReturningFrameFromNativeveTracker()

Looks "Native" got some extra letters.

Comment 9

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/974015054395

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/974015054395

Comment 11

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929