632924 - TM: Assertion failure: frame entry 40 wasn't freed: _entries[i] == __null

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

This test case:
--
function g() {
}
function f() {
    "use strict";
    for(var j=0; j<10; j++) {
        typeof(arguments);
        arguments.isPrototypeOf(g);
    }
}

for(var i=0; i<20; i++) {
    f(g);
}
--
Triggers this assert with -j:

Assertion failure: frame entry 40 wasn't freed
: _entries[i] == __null (../nanojit/Assembler.cpp:2335)

Comment 1

[Jan de Mooij [:jandem]]

Reduced:
--
function f() {
    "use strict";
    for(var j=0; j<10; j++) {
        var a = arguments;
        arguments.toString();
    }
}
f(2);
--

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Stacktrace

Attached stack trace is for this code:
--
function f() {
    "use strict"; 
    for(var j=0; j<20; j++) {
        var a = arguments;
        a = arguments;
    }
}
f(2);
--

Comment 3

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   49755:bf52361e6fd0
user:        Jeff Walden
date:        Wed Aug 11 23:27:33 2010 -0700
summary:     Bug 516255 - Eagerly copy initial parameter values into the arguments object when a function's parameters might be mutated, and rely on normal resolution behavior in the remaining cases when parameters are never modified.  r=dmandelin

Comment 4

[Nicholas Nethercote [inactive]]

This is an assertion in Nanojit and bug 516255's changeset didn't touch Nanojit, so it probably just exposed a latent bug in Nanojit.

Comment 5

[Nicholas Nethercote [inactive]]

It's a bug in TM.  Here's the relevant LIR:

  ------------------------------ # JSOP_ARGUMENTS
      $stack3 = ldi.sp sp[-32]
      $stack0 = ldi.sp sp[-56]
      allocp2 = allocp 4
      eqi4 = eqi $stack3, immi6/*0*/
      jt eqi4 -> label5
      sti.alloc allocp2[0] = $stack3
      j -> label4
      label5:
      globalObj = immi 0xf6d02028
      cx->fp()->numActualArgs() = immi 1
      js_NewArgumentsOnTrace2 = calli.all #js_NewArgumentsOnTrace ( cx globalObj/*0xf6d02028*/ cx->f
p()->numActualArgs()/*1*/ $stack0 )
      immi5 = immi 0
      eqi3 = eqi js_NewArgumentsOnTrace2, immi5/*0*/
      xt2: xt eqi3 -> exit=0x92693d4 pc=0x9263457 imacpc=(nil) sp+0 rp+0 OOM
      slots = ldi.objslots js_NewArgumentsOnTrace2[36]
      ldi6 = ldi.slots/c slots[8]
XXX   ldi3 = ldi.sp sp[-40]
      JSVAL_TAG_INT32 = immi 0xffff0001
      sti.argsdata ldi6[12] = JSVAL_TAG_INT32/*0xffff0001*/
      sti.argsdata ldi6[8] = ldi3
      sti.alloc allocp2[0] = js_NewArgumentsOnTrace2
      label4:
      ldi5 = ldi.alloc allocp2[0]
      sti.sp sp[-32] = ldi5
  ------------------------------ # JSOP_SETLOCAL
      sti.sp sp[-8] = ldi5
  ------------------------------ # JSOP_ARGUMENTS
      allocp1 = allocp 4
      immi4 = immi 0
      eqi2 = eqi ldi5, immi4/*0*/
      jt eqi2 -> label3
      sti.alloc allocp1[0] = ldi5
      j -> label2
      label3:
      globalObj2 = immi 0xf6d02028
      cx->fp()->numActualArgs()2 = immi 1
      js_NewArgumentsOnTrace1 = calli.all #js_NewArgumentsOnTrace ( cx globalObj2/*0xf6d02028*/ cx->fp()->numActualArgs()2/*1*/ $stack0 )
      immi3 = immi 0
      eqi1 = eqi js_NewArgumentsOnTrace1, immi3/*0*/
      xt1: xt eqi1 -> exit=0x926941c pc=0x926345c imacpc=(nil) sp+0 rp+0 OOM
      slots2 = ldi.objslots js_NewArgumentsOnTrace1[36]
      ldi4 = ldi.slots/c slots2[8]
XXX   # no ldi sp[-40] here
      JSVAL_TAG_INT32_2 = immi 0xffff0001
      sti.argsdata ldi4[12] = JSVAL_TAG_INT32_2/*0xffff0001*/
XXX   sti.argsdata ldi4[8] = ldi3
      sti.alloc allocp1[0] = js_NewArgumentsOnTrace1
      label2:
      ldi2 = ldi.alloc allocp1[0]
      sti.sp sp[-32] = ldi2

ldi3 is defined in one arm of a control-flow diamond for the first JSOP_ARGUMENTS.  Then the second JSOP_ARGUMENTS bogusly reused ldi3 instead of doing its own load.  I don't yet know why it does this.  At first I thought it might be a CSE bug but I turned CSE off and still got the assertion.

I'm marking this as [sg:critical?] because it could lead to garbage values being read and/or written to registers.

Comment 6

[Nicholas Nethercote [inactive]]

The problem seems to be with the Tracker -- a LIR instruction gets associated with a jsval.  This is normally fine, but in this case it's occurring within a control-flow diamond, which means that the Tracker entry becomes invalid once we leave the diamond.  But there's no way to clear the Tracker entry AFAIK.  So in the second diamond we bogusly reuse the LIR instruction again.

I'm not sure how to fix this.  Are we just doing something the tracer doesn't really support, ie. putting too much stuff in a diamond?  Does this problem occur elsewhere?

Comment 7

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

> This is normally fine, but in this case it's occurring within a control-flow diamond

What code is doing this? Setting the tracker inside a conditional branch is illegal. IIRC the first place we discovered such a bug was arguments-related (of course, what tracer bug isn't?) and that's why it uses stalloc/ldalloc now.

Comment 8

[Andreas Gal :gal]

Yeah, you have to update the tracker after the diamond.

Comment 9

[Jan de Mooij [:jandem]]

Assuming it's the same bug, this one crashes in a release build:
--
function f(o) {
    "use strict"; 
    for(var j=0; j<10; j++) {
        arguments.prototype;
        Object.getOwnPropertyNames(o);
    }
}
f(print);
--
--

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> 
> What code is doing this? 

The stack trace is this:

 Tracer::set()
 TraceRecorder::importImpl()
 TraceRecorder::getImpl()
 TraceRecorder::get()
 BoxArg::operator()
 JSStackFrame::forEachCanonicalActualArg()
 TraceRecorder::newArgument()
 TraceRecorder::record_JSOP_ARGUMENTS()

Comment 11

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

Jeff, this is definitely the strict-mode-arguments thing then. Aborting the trace would be easiest, but another option is to force a call to TR::get() for each canonical arg, right here:

    } else {
        // Generate LIR to create arguments only if it has not already been created.

        LIns* mem_ins = w.allocp(sizeof(JSObject *));

That will ensure that no imports happen inside a conditional branch.

Comment 12

[Nicholas Nethercote [inactive]]

I'd like to add a check to prevent imports from happening inside a conditional branch.  Shouldn't be too hard, just add a boolean to TraceRecorder.

Comment 13

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

That sounds like a good idea. In the meantime, can we just abort the trace for strict-mode arguments? Should be extremely low risk and easy sg:crit fix for 4.0.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> That sounds like a good idea. In the meantime, can we just abort the trace for
> strict-mode arguments? Should be extremely low risk and easy sg:crit fix for
> 4.0.

Sounds fine for fx4.

/be

Comment 15

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

Attachment: fix

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 513321 [details] [diff] [review]
fix

Can you file a follow-up bug for a proper fix?  Thanks.

Comment 17

[Andreas Gal :gal]

Can we file a bug to fix this post 4?

Comment 18

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

Sure, though I'd be happier to stop tracing anything related to arguments objects at all, a bug about it should at least be on file.

Comment 19

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

http://hg.mozilla.org/tracemonkey/rev/627635529dbe

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/627635529dbe