1323854 - Assertion failure: exprStack == stackDepth, at js/src/jit/Recover.cpp:110

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

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.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

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?

Comment 3

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

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

Comment 4

[Nicolas B. Pierron [:nbp]]

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.

Comment 5

[Hannes Verschore [:h4writer]]

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.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Patch

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 7

[Nicolas B. Pierron [:nbp]]

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?

Comment 8

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

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".

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Keep the same symantics as bytecode on CFGCompare

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 10

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Use a more correct pc for backedges in empty blocks

Comment 11

[Nicolas B. Pierron [:nbp]]

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 12

[Nicolas B. Pierron [:nbp]]

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.

Comment 13

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Keep the same symantics as bytecode on CFGCompare

Comment 14

[Hannes Verschore [:h4writer]]

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

Comment 15

[Nicolas B. Pierron [:nbp]]

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*);

Comment 16

[Hannes Verschore [:h4writer]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a2819ba15312

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Daniel Veditz [:dveditz]]

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!

Comment 20

[Hannes Verschore [:h4writer]]

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

Comment 21

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.