1116143 - BaselineDebugModeOSR handles some callVMs incorrectly

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

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 p.foo, true);
}
test();

Comment 1

[Shu-yu Guo [: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.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Patch bare callVMs correctly in debug mode OSR.

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.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Patch bare callVMs correctly in debug mode OSR.

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.

Comment 4

[Jan de Mooij [:jandem]]

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...

Comment 5

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba42b1966bc

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ba42b1966bc