Closed Bug 570663 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta4+

People

(Reporter: gkw, Assigned: njn)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

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)
I can't reproduce this with my macosx tm dbg shell.
Summary: TM: "Assertion failed: "Constantly false guard detected": 0 (" → TM: 'Assertion failed: "Constantly false guard detected": 0'
Windows-only!?
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)
kahoolawe:src gal$ ./Darwin_DBG.OBJ/js -j
js> for (var a = 0; a < 4; a++) {
    switch (NaN) {}
}
js>
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
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)
Blocks: 527512
Never mind, I can reproduce on Mac too after updating.
This is appearing quite-so-often in jsfunfuzz logs.
blocking2.0: --- → ?
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.
Assignee: general → nnethercote
blocking2.0: ? → beta4+
(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?
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?
(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.
Attached patch PatchSplinter Review
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.
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.
Nick, if either patch looks right to you, feel free to r+ it and land.
Comment on attachment 466517 [details] [diff] [review]
Patch

The second patch is better!
Attachment #466517 - Flags: review-
Comment on attachment 466520 [details] [diff] [review]
Patch based on no-op idea

Thanks!
Attachment #466520 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/f9ec16f32116
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug570663-2.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.