Closed Bug 1132584 Opened 10 years ago Closed 10 years ago

Assertion failure: Modified registers between VM call and OsiPoint, at jit/MacroAssembler.cpp

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

enableOsiPointRegisterChecks() function f() {} f.__defineGetter__("x", (function() { this._ })) for (var i = 0; i < 3; i++) { (function() { for (var j = 0; j < 1; j++) { f.x + 1 } })() } asserts js debug shell on m-c changeset 81f979b17fbd with --fuzzing-safe --no-threads --ion-eager at Assertion failure: Modified registers between VM call and OsiPoint, at jit/MacroAssembler.cpp. Debug configure options: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-optimize --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r 81f979b17fbd autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/d96d552ff899 user: Jan de Mooij date: Wed Feb 11 14:42:01 2015 +0100 summary: Bug 1129382 - Add Ion ICs for scripted getters/setters. r=efaust,nbp,djvj Jan, is bug 1129382 a likely regressor?
Flags: needinfo?(jdemooij)
This is a SIGTRAP, setting s-s by default.
Group: core-security
Attached file stack (deleted) —
(lldb) bt * thread #1: tid = 0x47816, 0x000000010403626a, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0) * frame #0: 0x000000010403626a (lldb) dis --start $pc-100 --end $pc 0x104036206: andb $0x68, %al 0x104036208: movsd 0x60(%rsp), %xmm12 0x10403620f: movsd 0x58(%rsp), %xmm11 0x104036216: movsd 0x50(%rsp), %xmm10 0x10403621d: movsd 0x48(%rsp), %xmm9 0x104036224: movsd 0x40(%rsp), %xmm8 0x10403622b: movsd 0x38(%rsp), %xmm7 0x104036231: movsd 0x30(%rsp), %xmm6 0x104036237: movsd 0x28(%rsp), %xmm5 0x10403623d: movsd 0x20(%rsp), %xmm4 0x104036243: movsd 0x18(%rsp), %xmm3 0x104036249: movsd 0x10(%rsp), %xmm2 0x10403624f: movsd 0x8(%rsp), %xmm1 0x104036255: movsd (%rsp), %xmm0 0x10403625a: addq $0x78, %rsp 0x10403625e: popq %rax 0x10403625f: popq %rcx 0x104036260: popq %rdx 0x104036261: popq %rsi 0x104036262: popq %rdi 0x104036263: popq %r8 0x104036265: popq %r9 0x104036267: popq %r10 0x104036269: int3 (lldb)
Not security sensitive; it's a problem with the OsiPoint register checks.
Group: core-security
Flags: needinfo?(jdemooij)
Attached patch Patch (deleted) — Splinter Review
The problem was: (1) We made a call from an Ion IC getter stub. (2) The getter script then did a callVM. We stored the live registers and did the OsiPoint register checks but left the JitActivation::checkRegs flags set to 1. (3) Then we returned to the getter IC stub, and after the IC we verified the live registers, but of course this doesn't match the registers the callee dumped. This wasn't a problem before because LCallGeneric/LCallKnown have no live registers. The ICs can make scripted calls from instructions with live registers (but these are saved of course). I fixed it by making sure we set checkRegs = 0 when we verify the registers.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8564116 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8564116 [details] [diff] [review] Patch Review of attachment 8564116 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +1778,5 @@ > + // Do this first, to ensure we always reset the checkRegs flag on the > + // JitActivation. > + if (shouldVerifyOsiPointRegs(safepoint)) > + verifyOsiPointRegs(safepoint); > +#endif To be honest, I would prefer if we could keep this code below the `markOsiPoint` function call. Instead, add the following line in the bailout / exception path instead: activation->setCheckRegs(false);
Attachment #8564116 - Flags: review?(nicolas.b.pierron) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: