Closed Bug 1116143 Opened 8 years ago Closed 8 years ago

BaselineDebugModeOSR handles some callVMs incorrectly


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: jandem, Unassigned)



(1 file, 1 obsolete file)

The debug mode OSR code assumes the only Baseline callVMs that can toggle debug mode are the interrupt handler, debugger statements or throw, but DELPROP and friends also use direct callVMs and can enable/disable the debugger as well, see the testcase below for instance:

$ js ~/dev/test.js
test.js:13:4 Error: Assertion failed: got ({}), expected true

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, true);
Flags: needinfo?(shu)
Great find.

I think the right fix here would be to resume at the return-from-callVM address instead of the next PC's code address after the debug mode OSR continuation fixer.
Flags: needinfo?(shu)
The reason to distinguish for-op and non-op callVM entries is so that when
we're patching a frame settled on pc offset 0, we don't end up patching a
return to some callVM in the prologue.
Attachment #8542321 - Flags: review?(jdemooij)
Fix the x86 crash, which was due to R0 containing ReturnReg, so we can't
restore R0 and ReturnReg at the same time.

Generate 2 tails for the debug mode OSR handler stub: one for returning from
callVMs, which will need to restore ReturnReg but not R0/R1, and all other
cases, which will need to restore R0/R1 but not ReturnReg.
Attachment #8542321 - Attachment is obsolete: true
Attachment #8542321 - Flags: review?(jdemooij)
Attachment #8545184 - Flags: review?(jdemooij)
Comment on attachment 8545184 [details] [diff] [review]
Patch bare callVMs correctly in debug mode OSR.

Review of attachment 8545184 [details] [diff] [review]:

Sorry for the delay, looks good.

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +996,5 @@
> +    //
> +    // 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
> +    // NUNBOX32 platforms R0 contains ReturnReg.

Nit: R1 contains ReturnReg (R0 is ecx/edx, R1 eax/edx and ReturnReg is eax). Also this is x86 only. Also not all NUNBOX32 platforms, on ARM it's R2 that contains r0...
Attachment #8545184 - Flags: review?(jdemooij) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.