Closed Bug 858749 Opened 9 years ago Closed 9 years ago

OdinMonkey: Assertion failure: predecessors_[j]->getSlot(i) == mine

Categories

(Core :: JavaScript Engine, defect)

22 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- fixed
firefox23 --- fixed

People

(Reporter: bc, Assigned: luke)

References

()

Details

(Keywords: assertion)

Attachments

(2 files)

Attached file crash report
1. http://n3emscripten.appspot.com/dragons_asmjs.html
2. wait a minute
3. Assertion failure: predecessors_[j]->getSlot(i) == mine, at c:/work/mozilla/builds/nightly/mozilla/js/src/ion/MIRGraph.cpp:677

js::ion::MBasicBlock::addPredecessorPopN js::ion::MBasicBlock::addPredecessor FunctionCompiler::bindBreaksOrContinues FunctionCompiler::bindContinues CheckWhile

Aurora/22, Nightly/23 - Windows, Mac, Linux
Summary: Assertion failure: predecessors_[j]->getSlot(i) == mine → OdinMonkey: Assertion failure: predecessors_[j]->getSlot(i) == mine
Assignee: general → luke
Attached patch fix and testSplinter Review
Great find!

I was incorrectly using "is the MBasicBlock" empty to detect whether a join block had already been created when binding breaks/continues; this misses the case where no instruction is added by the SSA state has been updated.  EIBTI once again.
Attachment #738738 - Flags: review?(sstangl)
s/by the SSA state/but the SSA state/
Comment on attachment 738738 [details] [diff] [review]
fix and test

Review of attachment 738738 [details] [diff] [review]:
-----------------------------------------------------------------

Just as a thought, it might be nice to take an MBasicBlock** instead of bool*, then, if the join block is provided, explicitly refer to it instead of using curBlock_. When refactoring IonBuilder, clarity was often improved by just naming blocks instead of using |current|.

::: js/src/ion/AsmJS.cpp
@@ +2232,1 @@
>      {

Although the code is obviously correct as-is, it may be nice to leave |JS_ASSERT(*createdJoinBlock == false)| as a signal to the reader that the initial value is expected to have been set by the caller.
Attachment #738738 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/d061ec6e8451
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 738738 [details] [diff] [review]
fix and test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 840282
User impact if declined: incorrect asm.js evaluation
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low
Attachment #738738 - Flags: approval-mozilla-aurora?
Attachment #738738 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.