The following testcase crashes on mozilla-central revision 489f981e8c2b (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager --ion-gvn=off): // Adapted from randomly chosen test: js/src/jit-test/tests/debug/Script-isInCatchScope.js var g = newGlobal(); var dbg = new Debugger(g); dbg.onExceptionUnwind = function(m) { do { m = m.older; } while (m != null); }; g.eval("try { throw (function() {});} finally {}"); Backtrace: 0 js-dbg-64-dm-clang-darwin-489f981e8c2b 0x0000000106446a99 js::jit::MResumePoint::writeRecoverData(js::jit::CompactBufferWriter&) const + 969 (Recover.cpp:110) 1 js-dbg-64-dm-clang-darwin-489f981e8c2b 0x00000001064847e5 js::jit::RecoverWriter::writeInstruction(js::jit::MNode const*) + 21 (Snapshots.cpp:722) 2 js-dbg-64-dm-clang-darwin-489f981e8c2b 0x00000001064c49bb js::jit::CodeGeneratorShared::encode(js::jit::LRecoverInfo*) + 203 (CodeGenerator-shared.cpp:569) 3 js-dbg-64-dm-clang-darwin-489f981e8c2b 0x00000001064c4b01 js::jit::CodeGeneratorShared::encode(js::jit::LSnapshot*) + 49 (CodeGenerator-shared.cpp:584) 4 js-dbg-64-dm-clang-darwin-489f981e8c2b 0x000000010654f79f void js::jit::CodeGeneratorX86Shared::bailout<js::jit::BailoutLabel>(js::jit::BailoutLabel const&, js::jit::LSnapshot*) + 31 (JitFrames.h:257) /snip For detailed crash information, see attachment.

autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/3daa33144b64 user: Hannes Verschore date: Thu Dec 08 13:53:05 2016 -1000 summary: Bug 1310155 - IonMonkey, part 1.0: Split graph creation from IonBuilder, r=jandem Hannes, is bug 1310155 a likely regressor?

Note that this seems to be different from other assertion failures with similar messages as this seems to require --ion-gvn=off.

This error message usually means that the resume point does not exactly capture the stack used by Baseline / Interpreter. Thus, using the snapshot which uses this resume point will probably cause mismatch when resuming the execution in Baseline.

The issue is that the backedge block (which is usually empty) points to a the last pc of the loop: > Dumping cfg: > > Block 0, 0:0 > LoopEntry [1] > > Block 1, 16:22 > Test [3, 4] > > Block 3, 22:22 > BackEdge [1] > > Block 4, 27:28 > 27: jumptarget > RetRVal [] But we manage to add a MFilterTypeSet and MUnbox in the backedge block. As a result we use pc:22 to encode the bailout, which doesn't work. Since that is the start of the block outside loop. As a result that is faulty. I think the easiest fix would be to adjust the pc to the loopEntry pc.

Update the "pc" to be the start of the loop if the block start/stop pc points to the same pc, which is actually after the loop. By pointing the the start of the loop, we can use that pc correctly for bailing.

Comment on attachment 8820307 [details] [diff] [review] Patch Review of attachment 8820307 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +13027,5 @@ > jsbytecode* pc() const { > return pc_; > } > + void updatePc(jsbytecode* pc) { > + pc_ = pc; The pc of the resume point is supposed to match the stack depth of the emulated interpreter stack (MBasicBlock::push & MBasicBlock::pop within IonBuilder) Thus mutating the pc after the allocation of the FixedList operands sounds like an error. The pc should remain constant once the ResumePoint is created. Using the Loop Entry PC is the right approach to this problem, as long as the backedge block is empty. However, I would recommend to do that as part of the creation of the backedges block, in IonControlFlow.cpp. Also, I wonder why this is a problem now, and why it was not there before?

Another testcase: g = (function () { "use asm"; function f(i0, d1) { d1 = 4258326539 >>> 0; switch (-8 && i0) { case -1: d1 = 0; case 0: } } return f; })(); g(); g(); This one involves "use asm".

This the issue Gary reported. In IonControlFlow we were having a block after the "case" (which pops 2) which in IonControlFlow we only pop 1 goto a empty block and pop again. For that empty block we needed an "pc", but there is no corresponding PC. That worked all right, but one of our passes is adding fallible functions in that empty block. Hey presto we got this issue. I fixed it by making sure CFGCompare directly pops 2 values. I still need the empty blocks, since "visitCompare" in IonBuilder cannot handle an already existing block. That block gets cleaned up later anyhow. As a result I don't see the need to fix that. If you think it would be better it should be a follow-up bug, since I wouldn't be surprised that introduced another bug ;)

Comment on attachment 8824943 [details] [diff] [review] Part 1: Use a more correct pc for backedges in empty blocks Review of attachment 8824943 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/bug1323854-2.js @@ +1,1 @@ > +// |jit-test| --ion-gvn=off; Exception note, do not land the test case with the patch if this is on aurora, also do we throw an Exception while the test case has a try-catch ?

Comment on attachment 8824448 [details] [diff] [review] Part 2: Keep the same symantics as bytecode on CFGCompare Review of attachment 8824448 [details] [diff] [review]: ----------------------------------------------------------------- Would it make more sense to have a CFGCase (-2, -1) and a CFGDefault (-2) similar to how they are implemented in the interpreter as JSOP_CASE and JSOP_DEFAULT? ::: js/src/jit/IonBuilder.cpp @@ +2807,5 @@ > AbortReasonOr<Ok> > IonBuilder::visitCompare(CFGCompare* compare) > { > + MDefinition* lhs = current->peek(-1); > + MDefinition* rhs = current->peek(-2); existing issue: -2 is lhs, and -1 is rhs.

Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2819ba153127179602ef73c9fb593250ae21167

Comment on attachment 8826145 [details] [diff] [review] Part 2: Keep the same symantics as bytecode on CFGCompare Review of attachment 8826145 [details] [diff] [review]: ----------------------------------------------------------------- nit: s/symantic/semantic/ in the patch subject. ::: js/src/jit/IonControlFlow.cpp @@ +131,5 @@ > CFGCompare* old = ins->toCompare(); > CFGBlock* trueBranch = &blocks_[old->trueBranch()->id()]; > CFGBlock* falseBranch = &blocks_[old->falseBranch()->id()]; > + copy = CFGCompare::New(alloc, trueBranch, old->truePopAmount(), > + falseBranch, old->falsePopAmount()); nit: I would much prefer if we could let the CFGCompare constructor copy the pop amount instead of giving them as argument here. ::: js/src/jit/IonControlFlow.h @@ +323,5 @@ > */ > class CFGCompare : public CFGAryControlInstruction<2> > { > + size_t truePopAmount_; > + size_t falsePopAmount_; maybe-nit: "const size_t …;" ? @@ +329,1 @@ > CFGCompare(CFGBlock* succ1, CFGBlock* succ2) This constructor is unused, remove it. @@ +343,5 @@ > } > > public: > CFG_CONTROL_HEADER(Compare); > TRIVIAL_CFG_NEW_WRAPPERS Replace the TRIVIAL_CFG_NEW_WRAPPER, as we do not want to be able to set the pop amount manually. Instead add the following function: static CFGCompare* CopyWithNewTargets(TempAllocator&, CFGCompare*, CFGBlock*, CFGBlock*);

Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/4dbe893355f7096535a833eeb314267f4258933a

https://hg.mozilla.org/mozilla-central/rev/4dbe893355f7 https://hg.mozilla.org/mozilla-central/rev/61d2cbe7cd8c https://hg.mozilla.org/mozilla-central/rev/b0ce1a6d05ed

Hannes: Can I assume that Gary was right about the regressor and that this affects only 53? Otherwise please retroactive fill out the sec-approval questions so we can evaluate whether we need to back-port this. Thanks!

Yes it only affects 53. Regression range was correct. (bug 1310155 which landed in FF53).

JSBugMon: This bug has been automatically verified fixed.