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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Jan de Mooij [:jandem]]

Ugh, good find.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 3

[Ted Campbell [:tcampbell]]

Do we need to handle ReturnDoubleReg as well? https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/js/src/jit/x64/Trampoline-x64.cpp#726,736,742

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 5

[Ted Campbell [: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.

Comment 6

[Brian Hackett [Laid off!]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1c57557089
Save/restore JSReturnOperand when doing debug mode OSR after a VM call returns, r=tcampbell.

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9a1c57557089