All users were logged out of Bugzilla on October 13th, 2018

BaselineDebugModeOSR handles some callVMs incorrectly

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Unassigned)

Tracking

unspecified
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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();
Flags: needinfo?(shu)

Comment 1

4 years ago
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)

Comment 2

4 years ago
Created attachment 8542321 [details] [diff] [review]
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.
Attachment #8542321 - Flags: review?(jdemooij)

Comment 3

4 years ago
Created attachment 8545184 [details] [diff] [review]
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.
Attachment #8542321 - Attachment is obsolete: true
Attachment #8542321 - Flags: review?(jdemooij)
Attachment #8545184 - Flags: review?(jdemooij)
(Reporter)

Comment 4

4 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/2ba42b1966bc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.