518492 - TM: fix and clarify incorrect STATUS_EXIT uses

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

After bug 518258 I put an assertion in the LeaveTree deep-bail case that after bumping regs.pc:

> JS_ASSERT(regs->pc == innermost->pc);

This assert hit in the following tests:
/Users/dvander/mozilla/safemonkey/js/src/trace-test/tests/basic/testInterpreterReentry6.js
/Users/dvander/mozilla/safemonkey/js/src/trace-test/tests/basic/testInterpreterReentry7.js

The culprits are these two functions, respectively:
 > TraceRecorder::initOrSetPropertyByName
 > TraceRecorder::initOrSetPropertyByIndex

They are taking pre-state STATUS_EXIT snapshots. LeaveTree wants a post-state snapshot, so the assertion fails. Problem is, the interpreter coalesces JSOP_SETELEM and JSOP_POP pairs. If we take a post-state snapshot, it's after the JSOP_POP, and the assertion _still_ fails because LeaveTree's regs->pc adjustment doesn't do the same coalescing.

What happens right now: If |SetPropertyByBleh| deep-bails, the second call to LeaveTree will push a bogus value on the stack, like bug 518258. If there's a JSOP_POP it'll just get popped.

The easiest thing to do here is to fix when the snapshot is taken, then do a fixup in LeaveTree so we can have a working assertion, and comment on this craziness.

Any objections?

Comment 1

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

Attachment: patch

Comment 2

[Andreas Gal :gal]

Comment on attachment 402511 [details] [diff] [review]
patch

Looks good to me. This is really bizarre behavior. The fusing of opcodes has bit us before.

Comment 3

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

http://hg.mozilla.org/tracemonkey/rev/0dfaa6e65031

Comment 4

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/967eebe275bc

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0dfaa6e65031

Comment 6

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/219cec621f3b

Comment 7

[Robert Sayre]

this isn't fixed 192, that changelog is for another patch that has the wrong bug number in it.