Closed Bug 1510105 Opened 9 months ago Closed 9 months ago

Save/restore JSReturnOperand when doing debug mode OSR after a VM call returns

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
The comment below, in EmitBaselineDebugModeOSRHandlerTail, looks wrong.

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

When a callVM returns, there are additional live registers besides ReturnReg: if the VM call has a Value outparam, that outparam will be in JSReturnOperand.  (The VM call can also write to ReturnDoubleReg, but that probably won't get clobbered before the handler tail finishes its job.)  By not saving/restoring JSReturnOperand here, its contents will be corrupted when the tail finishes and callers will behave incorrectly --- this problem manifested as a recording mismatch when using web replay due to the caller behaving differently in executions where the OSR occurred.  The symptoms here are also similar to those in bug 1485818.
Attachment #9027777 - Flags: review?(tcampbell)
Ugh, good find.
Attached patch patch (obsolete) — Splinter Review
The first patch turned out to be incomplete, JSReturnOperand may already have been clobbered by the time we get to the OSR handler tail code.
Attachment #9027777 - Attachment is obsolete: true
Attachment #9027777 - Flags: review?(tcampbell)
Attachment #9028139 - Flags: review?(jdemooij)
Attached patch patchSplinter Review
Yeah, the ABI permits the C++ functions called during OSR to clobber ReturnDoubleReg even if there isn't any floating point math in those functions.
Attachment #9028139 - Attachment is obsolete: true
Attachment #9028139 - Flags: review?(jdemooij)
Attachment #9028152 - Flags: review?(tcampbell)
Comment on attachment 9028152 [details] [diff] [review]
patch

Review of attachment 9028152 [details] [diff] [review]:
-----------------------------------------------------------------

Debug-OSR is very tricky code, but what your proposing makes sense and I'm not seeing anything that might blow up from this.

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +1108,5 @@
> +{
> +    // callVMs can use several different output registers, depending on the
> +    // type of their outparam.
> +    masm.push(ReturnReg);
> +    masm.push(ReturnDoubleReg);

I think this needs a |if (cx->runtime()->jitSupportsFloatingPoint)| guard.
Attachment #9028152 - Flags: review?(tcampbell) → review+
(In reply to Ted Campbell [:tcampbell] from comment #5)
> ::: js/src/jit/BaselineDebugModeOSR.cpp
> @@ +1108,5 @@
> > +{
> > +    // callVMs can use several different output registers, depending on the
> > +    // type of their outparam.
> > +    masm.push(ReturnReg);
> > +    masm.push(ReturnDoubleReg);
> 
> I think this needs a |if (cx->runtime()->jitSupportsFloatingPoint)| guard.

Per IRC discussion the jitSupportsFloatingPoint is not needed, as the baseline JIT is disabled if there is no floating point support and we only construct this handler if we need to do OSR on a baseline frame.
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1c57557089
Save/restore JSReturnOperand when doing debug mode OSR after a VM call returns, r=tcampbell.
https://hg.mozilla.org/mozilla-central/rev/9a1c57557089
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.