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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

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?

Comment 1

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

This is a SIGTRAP, setting s-s by default.

Comment 2

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

Attachment: stack

(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)

Comment 3

[Jan de Mooij [:jandem]]

Not security sensitive; it's a problem with the OsiPoint register checks.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 5

[Nicolas B. Pierron [:nbp]]

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

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b159a0401468

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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