570663 - TM: 'Assertion failed: "Constantly false guard detected": 0'

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

for (var a = 0; a < 4; a++) {
  switch (NaN) {}
}

asserts js debug shell on TM tip with -j at Assertion failed: "Constantly false guard detected": 0 (c:/jsfunfuzz-dbg-32-tm-43280-130b4376a842/compilePath/dbg-objdir/../nanojit/LIR.cpp:916)

Comment 1

[Andreas Gal :gal]

I can't reproduce this with my macosx tm dbg shell.

Comment 2

[Jesse Ruderman]

Windows-only!?

Comment 3

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

jsfunfuzz-dbg-32-tm-43271-9b2f825b2485 fuzz2$ ./js-dbg-32-tm-darwin -j
js> for (var a = 0; a < 4; a++) {
  switch (NaN) {}
}
Assertion failed: "Constantly false guard detected": 0 (../nanojit/LIR.cpp:916)

Comment 4

[Andreas Gal :gal]

kahoolawe:src gal$ ./Darwin_DBG.OBJ/js -j
js> for (var a = 0; a < 4; a++) {
    switch (NaN) {}
}
js>

Comment 5

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

Mac 32-bit debug TM changeset: 130b4376a842

js> for (var a = 0; a < 4; a++) {
  switch (NaN) {}
}
Assertion failed: "Constantly false guard detected": 0 (../nanojit/LIR.cpp:916)

Program received signal SIGABRT, Aborted.
0x93528132 in __kill ()
(gdb) bt
#0  0x93528132 in __kill ()
#1  0x93528124 in kill$UNIX2003 ()
#2  0x935ba8e5 in raise ()
#3  0x935d099c in abort ()
#4  0x001c3c7a in avmplus::AvmAssertFail () at ../nanojit/avmplus.cpp:65
#5  0x001bac3a in nanojit::ExprFilter::insGuard (this=0x86be84, v=nanojit::LIR_xf, c=0x868278, gr=0x8641bc) at ../nanojit/LIR.cpp:916
#6  0x00115e63 in nanojit::LirWriter::insGuard (this=0x86be8c, v=nanojit::LIR_xf, c=0x868278, gr=0x8641bc) at LIR.h:1443
#7  0x001b8950 in nanojit::ValidateWriter::insGuard (this=0x86be94, op=nanojit::LIR_xf, cond=0x868278, gr=0x8641bc) at ../nanojit/LIR.cpp:3197
#8  0x0019982b in js::TraceRecorder::tableswitch (this=0x867600) at ../jstracer.cpp:8463
#9  0x00199925 in js::TraceRecorder::record_JSOP_TABLESWITCH (this=0x867600) at ../jstracer.cpp:13452
#10 0x0019e104 in js::TraceRecorder::monitorRecording (this=0x867600, op=JSOP_TABLESWITCH) at jsopcode.tbl:193
#11 0x0008342e in js_Interpret (cx=0x809200) at jsops.cpp:78
#12 0x000a9767 in js_Execute (cx=0x809200, chain=0x1002000, script=0x40d060, down=0x0, flags=0, result=0xbffff688) at jsinterp.cpp:854
#13 0x00012758 in JS_ExecuteScript (cx=0x809200, obj=0x1002000, script=0x40d060, rval=0xbffff688) at ../jsapi.cpp:4608
#14 0x0000b384 in Process (cx=0x809200, obj=0x1002000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:515
#15 0x0000bd49 in ProcessArgs (cx=0x809200, obj=0x1002000, argv=0xbffff84c, argc=1) at ../../shell/js.cpp:836
#16 0x0000be5c in shell (cx=0x809200, argc=1, argv=0xbffff84c, envp=0xbffff854) at ../../shell/js.cpp:5018
#17 0x0000bf80 in main (argc=1, argv=0xbffff84c, envp=0xbffff854) at ../../shell/js.cpp:5107

Comment 6

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   43266:42fc5e98b20e
user:        Steven Johnson
date:        Thu Jun 03 10:35:26 2010 -0700
summary:     Bug 527512 - ExprFilter missing trivial optimizations for ugt, ult (r=edwsmith,nnethercote)

Comment 7

[Jesse Ruderman]

Never mind, I can reproduce on Mac too after updating.

Comment 8

[Edwin Smith]

does this fix it?  

http://hg.mozilla.org/projects/nanojit-central/rev/eb43593410b9

Comment 9

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

(In reply to comment #8)
> does this fix it?  
> 
> http://hg.mozilla.org/projects/nanojit-central/rev/eb43593410b9

Nope. That's http://hg.mozilla.org/tracemonkey/rev/9f626937ec45 and the issue still occurs in a later changeset:

http://hg.mozilla.org/tracemonkey/rev/130b4376a842

Comment 10

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

This is appearing quite-so-often in jsfunfuzz logs.

Comment 11

[Nicholas Nethercote [inactive]]

If the frequency has recently increased, it's probably due to the landing of bug 560106 which increases the amount of constant folding on 64-bit.

Comment 12

[Nicholas Nethercote [inactive]]

(Gal, dmandelin: you should read this)

The relevant code is in TraceRecorder::tableswitch():

    /* Generate switch LIR. */                                
    SwitchInfo* si = new (traceAlloc()) SwitchInfo();         
    si->count = high + 1 - low;                               
    si->table = 0;                                            
    si->index = (uint32) -1;                                  
    LIns* diff = lir->ins2(LIR_subi, v_ins, lir->insImmI(low));
    LIns* cmp = lir->ins2(LIR_ltui, diff, lir->insImmI(si->count));
    lir->insGuard(LIR_xf, cmp, createGuardRecord(snapshot(DEFAULT_EXIT)));
    lir->insStore(diff, lir->insImmP(&si->index), 0, ACCSET_OTHER);
    VMSideExit* exit = snapshot(CASE_EXIT);                   
    exit->switchInfo = si;                                    
    LIns* guardIns = lir->insGuard(LIR_xtbl, diff, createGuardRecord(exit));
    fragment->lastIns = guardIns;                             
    CHECK_STATUS_A(compile());                                
    return finishSuccessfully();  

In this case high=-1, low = 0, so si->count = 0.  Which means that 'cmp' is always false, because ltui(x, 0) is always false.

I tried testing if si->count is zero and avoided generating the guard in that case, but then I get a seg fault elsewhere.  I suspect that we should bail earlier if the switched-on value is bogus as in this case, but I don't know exactly how/when.  The high=-1 doesn't seem right, though, maybe we should check for that?

Gal, dmandelin, any ideas?

Comment 13

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

For a never-taken switch, it should be safe to elide the guard and make tableswitch() a nop, like JSOP_GOTO. In fact we already have a check earlier:

if (v_ins->isImmI())
    return ARECORD_CONTINUE;

It should be safe to check for isImmF() as well. Does that crash?

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #13)
> For a never-taken switch, it should be safe to elide the guard and make
> tableswitch() a nop, like JSOP_GOTO. In fact we already have a check earlier:
> 
> if (v_ins->isImmI())
>     return ARECORD_CONTINUE;
> 
> It should be safe to check for isImmF() as well. Does that crash?

v_ins is a LIR_calli to js_DoubleToInt32(), so checking for isImmF() makes no difference.

Comment 15

[David Mandelin [:dmandelin]]

Attachment: Patch

I think this is one way to fix it. The intent of the original LIR was this:

  1. Check if the switch expr is in bounds.
  2. If switch expr is out of bounds, take a DEFAULT_EXIT, which will cause
     another trace to get started at this point to handle the default case.
  3. Otherwise, take a LIR_xtbl CASE_EXIT, which will cause a new trace to
     get started at this point for that case if needed, otherwise it will just
     run it from the jump table.

The attached patch simply takes the DEFAULT_EXIT always, so a new trace will start and get attached to this point.

It would also make sense to turn the tableswitch into a no-op when there are no cases. I'll try that too and post a patch/results.

Comment 16

[David Mandelin [:dmandelin]]

Attachment: Patch based on no-op idea

This one also seems to work. I consider it better because it doesn't create an unnecessary separate trace. It is based on the fact that if there are no cases, then whatever op gets run next, whether it be in the default case or the following code, is always the op that gets done next. So no code is required for the switch.

Comment 17

[David Mandelin [:dmandelin]]

Nick, if either patch looks right to you, feel free to r+ it and land.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 466517 [details] [diff] [review]
Patch

The second patch is better!

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 466520 [details] [diff] [review]
Patch based on no-op idea

Thanks!

Comment 20

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/59344fee4da6

Comment 21

[Robert Sayre]

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

Comment 22

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug570663-2.js.