From 15a1da71fdc0a36c25b774c1aa00c56f0501de66 Mon Sep 17 00:00:00 2001 From: Shu-yu Guo Date: Wed, 7 Jan 2015 22:02:35 -0800 Subject: [PATCH] Bug 1116143 - Patch bare callVMs correctly in debug mode OSR. (r=jandem) --- .../tests/debug/execution-observability-03.js | 17 ++ js/src/jit/BaselineCompiler.cpp | 6 +- js/src/jit/BaselineDebugModeOSR.cpp | 152 +++++++++++------- js/src/jit/BaselineIC.h | 10 +- js/src/jit/BaselineJIT.cpp | 37 ++++- js/src/jit/BaselineJIT.h | 1 + js/src/jit/shared/BaselineCompiler-shared.h | 7 + 7 files changed, 165 insertions(+), 65 deletions(-) create mode 100644 js/src/jit-test/tests/debug/execution-observability-03.js diff --git a/js/src/jit-test/tests/debug/execution-observability-03.js b/js/src/jit-test/tests/debug/execution-observability-03.js new file mode 100644 index 000000000000..bc46d46d4c88 --- /dev/null +++ b/js/src/jit-test/tests/debug/execution-observability-03.js @@ -0,0 +1,17 @@ +// Tests that bare callVMs (in the delprop below) are patched correctly. + +var o = {}; +var global = this; +var p = new Proxy(o, { + "deleteProperty": function (target, key) { + var g = newGlobal(); + g.parent = global; + g.eval("var dbg = new Debugger(parent); dbg.onEnterFrame = function(frame) {};"); + return true; + } +}); +function test() { + for (var i=0; i<100; i++) {} + assertEq(delete p.foo, true); +} +test(); diff --git a/js/src/jit/BaselineCompiler.cpp b/js/src/jit/BaselineCompiler.cpp index ef0dafd0aa85..0dae386b55a7 100644 --- a/js/src/jit/BaselineCompiler.cpp +++ b/js/src/jit/BaselineCompiler.cpp @@ -551,7 +551,7 @@ BaselineCompiler::emitStackCheck(bool earlyCheck) else if (needsEarlyStackCheck()) phase = CHECK_OVER_RECURSED; - if (!callVM(CheckOverRecursedWithExtraInfo, phase)) + if (!callVMNonOp(CheckOverRecursedWithExtraInfo, phase)) return false; masm.bind(&skipCall); @@ -626,7 +626,7 @@ BaselineCompiler::initScopeChain() masm.loadBaselineFramePtr(BaselineFrameReg, R0.scratchReg()); pushArg(R0.scratchReg()); - if (!callVM(HeavyweightFunPrologueInfo, phase)) + if (!callVMNonOp(HeavyweightFunPrologueInfo, phase)) return false; } } else { @@ -640,7 +640,7 @@ BaselineCompiler::initScopeChain() masm.loadBaselineFramePtr(BaselineFrameReg, R0.scratchReg()); pushArg(R0.scratchReg()); - if (!callVM(StrictEvalPrologueInfo, phase)) + if (!callVMNonOp(StrictEvalPrologueInfo, phase)) return false; } } diff --git a/js/src/jit/BaselineDebugModeOSR.cpp b/js/src/jit/BaselineDebugModeOSR.cpp index 225f13328d4a..da8d6fb40a20 100644 --- a/js/src/jit/BaselineDebugModeOSR.cpp +++ b/js/src/jit/BaselineDebugModeOSR.cpp @@ -350,8 +350,7 @@ PatchBaselineFramesForDebugMode(JSContext *cx, const Debugger::ExecutionObservab // // Off to On: // A. From a "can call" stub. - // B. From a VM call (interrupt handler, debugger statement handler, - // throw). + // B. From a VM call. // H. From inside HandleExceptionBaseline. // // On to Off: @@ -473,25 +472,27 @@ PatchBaselineFramesForDebugMode(JSContext *cx, const Debugger::ExecutionObservab bool popFrameReg; switch (kind) { - case ICEntry::Kind_CallVM: + case ICEntry::Kind_CallVM: { // Case B above. // - // Patching returns from an interrupt handler or the debugger - // statement handler is similar in that we can resume at the - // next op. + // Patching returns from a VM call. After fixing up the the + // continuation for unsynced values (the frame register is + // popped by the callVM trampoline), we resume at the + // return-from-callVM address. The assumption here is that all + // callVMs which can trigger debug mode OSR are the *only* + // callVMs generated for their respective pc locations in the + // baseline JIT code. // - // Throws are treated differently, as patching a throw means - // we are recompiling on-stack scripts from inside an - // onExceptionUnwind invocation. The resume address must be - // settled on the throwing pc and not its successor, so that - // Debugger.Frame may report the correct offset. Note we never - // actually resume execution there, and it is set for the sake - // of frame iterators. - if (!iter.baselineFrame()->isDebuggerHandlingException()) - pc += GetBytecodeLength(pc); - recompInfo->resumeAddr = bl->nativeCodeForPC(script, pc, &recompInfo->slotInfo); - popFrameReg = true; + // Get the slot info for the next pc, but ignore the code + // address. We get the code address from the + // return-from-callVM entry instead. + (void) bl->maybeNativeCodeForPC(script, pc + GetBytecodeLength(pc), + &recompInfo->slotInfo); + ICEntry &callVMEntry = bl->callVMEntryFromPCOffset(pcOffset); + recompInfo->resumeAddr = bl->returnAddressForIC(callVMEntry); + popFrameReg = false; break; + } case ICEntry::Kind_DebugTrap: // Case C above. @@ -906,12 +907,9 @@ HasForcedReturn(BaselineDebugModeOSRInfo *info, bool rv) return true; // |rv| is the value in ReturnReg. If true, in the case of the prologue, - // debug trap, and debugger statement handler, it means a forced return. - if (kind == ICEntry::Kind_DebugPrologue || - (kind == ICEntry::Kind_CallVM && JSOp(*info->pc) == JSOP_DEBUGGER)) - { + // it means a forced return. + if (kind == ICEntry::Kind_DebugPrologue) return rv; - } // N.B. The debug trap handler handles its own forced return, so no // need to deal with it here. @@ -934,13 +932,19 @@ SyncBaselineDebugModeOSRInfo(BaselineFrame *frame, Value *vp, bool rv) return; } - // Read stack values and make sure R0 and R1 have the right values. - unsigned numUnsynced = info->slotInfo.numUnsynced(); - MOZ_ASSERT(numUnsynced <= 2); - if (numUnsynced > 0) - info->popValueInto(info->slotInfo.topSlotLocation(), vp); - if (numUnsynced > 1) - info->popValueInto(info->slotInfo.nextSlotLocation(), vp); + // Read stack values and make sure R0 and R1 have the right values if we + // aren't returning from a callVM. + // + // In the case of returning from a callVM, we don't need to restore R0 and + // R1 ourself since we'll return into code that does it if needed. + if (info->frameKind != ICEntry::Kind_CallVM) { + unsigned numUnsynced = info->slotInfo.numUnsynced(); + MOZ_ASSERT(numUnsynced <= 2); + if (numUnsynced > 0) + info->popValueInto(info->slotInfo.topSlotLocation(), vp); + if (numUnsynced > 1) + info->popValueInto(info->slotInfo.nextSlotLocation(), vp); + } // Scale stackAdjust. info->stackAdjust *= sizeof(Value); @@ -985,6 +989,53 @@ JitRuntime::getBaselineDebugModeOSRHandlerAddress(JSContext *cx, bool popFrameRe : baselineDebugModeOSRHandlerNoFrameRegPopAddr_; } +static void +EmitBaselineDebugModeOSRHandlerTail(MacroAssembler &masm, Register temp, bool returnFromCallVM) +{ + // Save real return address on the stack temporarily. + // + // If we're returning from a callVM, we don't need to worry about R0 and + // R1 but do need to propagate the original ReturnReg value. Otherwise we + // need to worry about R0 and R1 but can clobber ReturnReg. Indeed, on + // x86, R1 contains ReturnReg. + if (returnFromCallVM) { + masm.push(ReturnReg); + } else { + masm.pushValue(Address(temp, offsetof(BaselineDebugModeOSRInfo, valueR0))); + masm.pushValue(Address(temp, offsetof(BaselineDebugModeOSRInfo, valueR1))); + } + masm.push(BaselineFrameReg); + masm.push(Address(temp, offsetof(BaselineDebugModeOSRInfo, resumeAddr))); + + // Call a stub to free the allocated info. + masm.setupUnalignedABICall(1, temp); + masm.loadBaselineFramePtr(BaselineFrameReg, temp); + masm.passABIArg(temp); + masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, FinishBaselineDebugModeOSR)); + + // Restore saved values. + GeneralRegisterSet jumpRegs(GeneralRegisterSet::All()); + if (returnFromCallVM) { + jumpRegs.take(ReturnReg); + } else { + jumpRegs.take(R0); + jumpRegs.take(R1); + } + jumpRegs.take(BaselineFrameReg); + Register target = jumpRegs.takeAny(); + + masm.pop(target); + masm.pop(BaselineFrameReg); + if (returnFromCallVM) { + masm.pop(ReturnReg); + } else { + masm.popValue(R1); + masm.popValue(R0); + } + + masm.jump(target); +} + JitCode * JitRuntime::generateBaselineDebugModeOSRHandler(JSContext *cx, uint32_t *noFrameRegPopOffsetOut) { @@ -1005,6 +1056,7 @@ JitRuntime::generateBaselineDebugModeOSRHandler(JSContext *cx, uint32_t *noFrame // Record the stack pointer for syncing. masm.movePtr(StackPointer, syncedStackStart); + masm.push(ReturnReg); masm.push(BaselineFrameReg); // Call a stub to fully initialize the info. @@ -1016,37 +1068,27 @@ JitRuntime::generateBaselineDebugModeOSRHandler(JSContext *cx, uint32_t *noFrame masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, SyncBaselineDebugModeOSRInfo)); // Discard stack values depending on how many were unsynced, as we always - // have a fully synced stack in the recompile handler. See assert in - // DebugModeOSREntry constructor. + // have a fully synced stack in the recompile handler. We arrive here via + // a callVM, and prepareCallVM in BaselineCompiler always fully syncs the + // stack. masm.pop(BaselineFrameReg); + masm.pop(ReturnReg); masm.loadPtr(Address(BaselineFrameReg, BaselineFrame::reverseOffsetOfScratchValue()), temp); masm.addPtr(Address(temp, offsetof(BaselineDebugModeOSRInfo, stackAdjust)), StackPointer); - // Save real return address on the stack temporarily. - masm.pushValue(Address(temp, offsetof(BaselineDebugModeOSRInfo, valueR0))); - masm.pushValue(Address(temp, offsetof(BaselineDebugModeOSRInfo, valueR1))); - masm.push(BaselineFrameReg); - masm.push(Address(temp, offsetof(BaselineDebugModeOSRInfo, resumeAddr))); + // Emit two tails for the case of returning from a callVM and all other + // cases, as the state we need to restore differs depending on the case. + Label returnFromCallVM, end; + masm.branch32(MacroAssembler::Equal, + Address(temp, offsetof(BaselineDebugModeOSRInfo, frameKind)), + Imm32(ICEntry::Kind_CallVM), + &returnFromCallVM); - // Call a stub to free the allocated info. - masm.setupUnalignedABICall(1, temp); - masm.loadBaselineFramePtr(BaselineFrameReg, temp); - masm.passABIArg(temp); - masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, FinishBaselineDebugModeOSR)); - - // Restore saved values. - GeneralRegisterSet jumpRegs(GeneralRegisterSet::All()); - jumpRegs.take(R0); - jumpRegs.take(R1); - jumpRegs.take(BaselineFrameReg); - Register target = jumpRegs.takeAny(); - - masm.pop(target); - masm.pop(BaselineFrameReg); - masm.popValue(R1); - masm.popValue(R0); - - masm.jump(target); + EmitBaselineDebugModeOSRHandlerTail(masm, temp, /* returnFromCallVM = */ false); + masm.jump(&end); + masm.bind(&returnFromCallVM); + EmitBaselineDebugModeOSRHandlerTail(masm, temp, /* returnFromCallVM = */ true); + masm.bind(&end); Linker linker(masm); AutoFlushICache afc("BaselineDebugModeOSRHandler"); diff --git a/js/src/jit/BaselineIC.h b/js/src/jit/BaselineIC.h index 4a8a2c94b44a..60de69df253d 100644 --- a/js/src/jit/BaselineIC.h +++ b/js/src/jit/BaselineIC.h @@ -222,9 +222,13 @@ class ICEntry // A non-op IC entry. Kind_NonOp, - // A fake IC entry for returning from a callVM. + // A fake IC entry for returning from a callVM for an op. Kind_CallVM, + // A fake IC entry for returning from a callVM not for an op (e.g., in + // the prologue). + Kind_NonOpCallVM, + // A fake IC entry for returning from DebugTrapHandler. Kind_DebugTrap, @@ -299,6 +303,10 @@ class ICEntry MOZ_ASSERT(kind() == Kind_CallVM); setKind(Kind_DebugEpilogue); } + void setForNonOpCallVM() { + MOZ_ASSERT(kind() == Kind_CallVM); + setKind(Kind_NonOpCallVM); + } bool hasStub() const { return firstStub_ != nullptr; diff --git a/js/src/jit/BaselineJIT.cpp b/js/src/jit/BaselineJIT.cpp index 9383079d707d..584fed0c69ac 100644 --- a/js/src/jit/BaselineJIT.cpp +++ b/js/src/jit/BaselineJIT.cpp @@ -563,16 +563,14 @@ BaselineScript::returnAddressForIC(const ICEntry &ent) return method()->raw() + ent.returnOffset().offset(); } -ICEntry & -BaselineScript::icEntryFromPCOffset(uint32_t pcOffset) +static inline size_t +ComputeBinarySearchMid(BaselineScript *baseline, uint32_t pcOffset) { - // Multiple IC entries can have the same PC offset, but this method only looks for - // those which have isForOp() set. size_t bottom = 0; - size_t top = numICEntries(); + size_t top = baseline->numICEntries(); size_t mid = bottom + (top - bottom) / 2; while (mid < top) { - ICEntry &midEntry = icEntry(mid); + ICEntry &midEntry = baseline->icEntry(mid); if (midEntry.pcOffset() < pcOffset) bottom = mid + 1; else if (midEntry.pcOffset() > pcOffset) @@ -581,6 +579,15 @@ BaselineScript::icEntryFromPCOffset(uint32_t pcOffset) break; mid = bottom + (top - bottom) / 2; } + return mid; +} + +ICEntry & +BaselineScript::icEntryFromPCOffset(uint32_t pcOffset) +{ + // Multiple IC entries can have the same PC offset, but this method only looks for + // those which have isForOp() set. + size_t mid = ComputeBinarySearchMid(this, pcOffset); // Found an IC entry with a matching PC offset. Search backward, and then // forward from this IC entry, looking for one with the same PC offset which @@ -619,6 +626,24 @@ BaselineScript::icEntryFromPCOffset(uint32_t pcOffset, ICEntry *prevLookedUpEntr return icEntryFromPCOffset(pcOffset); } +ICEntry & +BaselineScript::callVMEntryFromPCOffset(uint32_t pcOffset) +{ + // Like icEntryFromPCOffset, but only looks for the fake ICEntries + // inserted by VM calls. + size_t mid = ComputeBinarySearchMid(this, pcOffset); + + for (size_t i = mid; i < numICEntries() && icEntry(i).pcOffset() == pcOffset; i--) { + if (icEntry(i).kind() == ICEntry::Kind_CallVM) + return icEntry(i); + } + for (size_t i = mid+1; i < numICEntries() && icEntry(i).pcOffset() == pcOffset; i++) { + if (icEntry(i).kind() == ICEntry::Kind_CallVM) + return icEntry(i); + } + MOZ_CRASH("Invalid PC offset for callVM entry."); +} + ICEntry * BaselineScript::maybeICEntryFromReturnAddress(uint8_t *returnAddr) { diff --git a/js/src/jit/BaselineJIT.h b/js/src/jit/BaselineJIT.h index 3f6fbeb83ded..c87be741045c 100644 --- a/js/src/jit/BaselineJIT.h +++ b/js/src/jit/BaselineJIT.h @@ -346,6 +346,7 @@ struct BaselineScript ICEntry &icEntryFromReturnOffset(CodeOffsetLabel returnOffset); ICEntry &icEntryFromPCOffset(uint32_t pcOffset); ICEntry &icEntryFromPCOffset(uint32_t pcOffset, ICEntry *prevLookedUpEntry); + ICEntry &callVMEntryFromPCOffset(uint32_t pcOffset); ICEntry *maybeICEntryFromReturnAddress(uint8_t *returnAddr); ICEntry &icEntryFromReturnAddress(uint8_t *returnAddr); uint8_t *returnAddressForIC(const ICEntry &ent); diff --git a/js/src/jit/shared/BaselineCompiler-shared.h b/js/src/jit/shared/BaselineCompiler-shared.h index e77715a691fb..fa7740bc2245 100644 --- a/js/src/jit/shared/BaselineCompiler-shared.h +++ b/js/src/jit/shared/BaselineCompiler-shared.h @@ -140,6 +140,13 @@ class BaselineCompilerShared }; bool callVM(const VMFunction &fun, CallVMPhase phase=POST_INITIALIZE); + bool callVMNonOp(const VMFunction &fun, CallVMPhase phase=POST_INITIALIZE) { + if (!callVM(fun, phase)) + return false; + icEntries_.back().setForNonOpCallVM(); + return true; + } + public: BytecodeAnalysis &analysis() { return analysis_;