Bug 150336 – REGRESSION (r191175): OSR Exit from an inlined tail callee trashes callee save registers

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

This is the cause for the crash described in <https://bugs.webkit.org/show_bug.cgi?id=150251> that was temporarily resolved by turning off tail calls.

Here is the relevant part of the backtrace:
* thread #1: tid = 0xb381ca, 0x000000011687b4be, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=EXC_I386)
    frame #0: 0x000000011687b4be JavaScriptCore`JSC::JITCode::execute(this=<unavailable>, vm=0xffff000000000000, protoCallFrame=0x00007fff50a830b8) + 158 at JITCode.cpp:81
    frame #1: 0x0000000116857dcf JavaScriptCore`JSC::Interpreter::executeCall(this=<unavailable>, callFrame=0x00007fff50a83220, function=0x000000011d836d70, callType=<unavailable>, callData=0x00007fff50a83180, thisValue=JSValue @ 0x00007fff50a83098, args=<unavailable>) + 447 at Interpreter.cpp:1024
    frame #2: 0x00000001164f99ce JavaScriptCore`JSC::call(exec=<unavailable>, functionObject=<unavailable>, callType=<unavailable>, callData=<unavailable>, thisValue=<unavailable>, args=<unavailable>) + 62 at CallData.cpp:39
    frame #3: 0x00000001168c409a JavaScriptCore`JSC::boundFunctionCall(exec=0x00007fff50a83220) + 618 at JSBoundFunction.cpp:54
    frame #4: 0x00003cff43e01028 0x3cff43e5c5a6
    frame #5: 0x00003cff43e5c5a6 shouldComponentUpdate#CiYug9 [Baseline](Cell[Object ID: 19284]: 0x1351bf340, Cell[Object ID: 17094]: 0x11ecfa9e0, Cell[Object ID: 19279]: 0x125946080, Cell[Object ID: 9671]: 0x13bd3c140)
    frame #6: 0x00003cff446f7c67 performUpdateIfNecessary#ADcVpe [DFG](Cell[Object ID: 19284]: 0x1351bf340, Cell[Object ID: 15608]: 0x11e863370)
    frame #7: 0x00003cff44219470 receiveComponent#D0omON [DFG](Cell[Object ID: 19284]: 0x1351bf340, Cell[Object ID: 15394]: 0x12e9ce200, Cell[Object ID: 15608]: 0x11e863370)
    frame #8: 0x00003cff4428f565 _updateChildren#D8I5sZ [DFG](Cell[Object ID: 15798]: 0x11dcf7400, Cell[Array ID: 184]: 0x12285a320, Cell[Object ID: 15608]: 0x11e863370)
    frame #9: 0x00003cff44294caf updateChildren#AAAS3a [DFG](Cell[Object ID: 15798]: 0x11dcf7400, Cell[Array ID: 184]: 0x12285a320, Cell[Object ID: 15608]: 0x11e863370)
...

The damage actually occurs before we get here.  The problem is that vm has a totally bad value (0xffff000000000000).  This happens to be the tagTypeNumber constant used by JSVALUE64 builds.  The top 4 frames of the stack show that we are calling a bound function.  The called function and its callee are:
hasFalkorPropChanged#CPIwPX: function(e) {
    "use strict";
    if (this.customModelChangeDetector)
        return this.customModelChangeDetector.didChange();
    var t = e.getBoundValue(),
        r = t.__generation,
        n = t.__key;
    return this.state.prevGeneration !== r || this.state.prevKey !== n
}

didChange#Agw6xZ: function() {
    "use strict";
    var e = this.model.getValueSync("length");
    return e !== this.prevModelLength ? !0 : this.isTransientRow() && this.generateUniqueKey() !== this.prevUniqueKey ? !0 : !1
}

generateUniqueKey#As9POD: function() {
    "use strict";
    for (var e, t = this.model.getValueSync("length"), n = "", i = 0; t > i; i++)
        e = this.model.getValueSync([i, "summary"]), e && e.id && (n += e.id.toString().substr(-4));
    return n
}

During DFG compilation, didChange() and generateUniqueKey() get inlined into hasFalkorPropChanged(), with didChange() in tail position and therefore made a tail call.  We OSR exit from generateUniqueKey() due to a structure change.  It appears that the OSR exit handler does not properly restore the callee saves registers for didChange(), which should be the callee saves for hasFalkorPropChanged().

[Michael Saboff]

Created attachment 263499 [details]
Reduced test case

[Michael Saboff]

Created attachment 263533 [details]
Patch

[Michael Saboff]

<rdar://problem/22951399>

[Mark Lam]

Comment on attachment 263533 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 263533 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263533&action=review

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:218
> +        // Restore the inline call frame's callee saves registers

callee save registers

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:229
> +        for (unsigned i = 0; i < registerCount; i++) {
> +            RegisterAtOffset entry = calleeSaves->at(i);
> +            if (dontSaveRegisters.get(entry.reg()))
> +                continue;
> +
> +            GPRReg registerToWrite;

This function is very similar to emitSaveCalleeSavesFor, with the only difference being the inline offset adjustment and the !trueCaller special case.

You should either factor this into a helper function that lives next to emitSaveCalleeSavesFor or parameterize emitSaveCalleeSavesFor to handle both cases.

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:234
> +            // If we need to restore a "tag" register for a tail callee that will be the top frame,
> +            // get the value saved for the outermost frame instead of the actual register which
> +            // has a tag constant.

This comment says what you did but not why. Why did you do this?

[Michael Saboff]

(In reply to comment #5)
> Comment on attachment 263533 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263533&action=review
> 
> > Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:218
> > +        // Restore the inline call frame's callee saves registers
> 
> callee save registers

Fixed.

> > Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:229
> > +        for (unsigned i = 0; i < registerCount; i++) {
> > +            RegisterAtOffset entry = calleeSaves->at(i);
> > +            if (dontSaveRegisters.get(entry.reg()))
> > +                continue;
> > +
> > +            GPRReg registerToWrite;
> 
> This function is very similar to emitSaveCalleeSavesFor, with the only
> difference being the inline offset adjustment and the !trueCaller special
> case.
> 
> You should either factor this into a helper function that lives next to
> emitSaveCalleeSavesFor or parameterize emitSaveCalleeSavesFor to handle both
> cases.

I refactored and moved it to be next to emitSaveCalleeSavesFor().

> > Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:234
> > +            // If we need to restore a "tag" register for a tail callee that will be the top frame,
> > +            // get the value saved for the outermost frame instead of the actual register which
> > +            // has a tag constant.
> 
> This comment says what you did but not why. Why did you do this?
Changed the comment to:
    // If this inlined frame is a tail call that will return back to the original caller, we need to
    // copy the prior contents of the tag registers already saved for the outer frame to this frame.

With the refactoring, I now have in AssemblyHelpers.h:

enum RestoreTagRegisterMode { UseExistingTagRegisterContents, CopySavedTagRegistersFromBaseFrame };

void emitSaveOrCopyCalleeSavesFor(CodeBlock* codeBlock, VirtualRegister offsetVirtualRegister, RestoreTagRegisterMode tagRegisterMode, GPRReg temp)
{   
    ASSERT(codeBlock);
        
    RegisterAtOffsetList* calleeSaves = codeBlock->calleeSaveRegisters();
    RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());
    unsigned registerCount = calleeSaves->size();
        
    for (unsigned i = 0; i < registerCount; i++) {
        RegisterAtOffset entry = calleeSaves->at(i);
        if (dontSaveRegisters.get(entry.reg()))
            continue;
            
        GPRReg registerToWrite;
            
#if USE(JSVALUE32_64)
        UNUSED_PARAM(tagRegisterMode);
        UNUSED_PARAM(temp);
#else
        if (tagRegisterMode == CopySavedTagRegistersFromBaseFrame
            && (entry.reg() == GPRInfo::tagTypeNumberRegister || entry.reg() == GPRInfo::tagMaskRegister)) {
            registerToWrite = temp;
            loadPtr(AssemblyHelpers::Address(GPRInfo::callFrameRegister, entry.offset()), registerToWrite);
        } else
#endif
            registerToWrite = entry.reg().gpr();

        storePtr(registerToWrite, Address(framePointerRegister, offsetVirtualRegister.offsetInBytes() + entry.offset()));
    }   
}   

and a call site of:
        // Restore the inline call frame's callee save registers.
        // If this inlined frame is a tail call that will return back to the original caller, we need to
        // copy the prior contents of the tag registers already saved for the outer frame to this frame.
        jit.emitSaveOrCopyCalleeSavesFor(
            baselineCodeBlock,
            static_cast<VirtualRegister>(inlineCallFrame->stackOffset),
            trueCaller ? AssemblyHelpers::UseExistingTagRegisterContents : AssemblyHelpers::CopySavedTagRegistersFromBaseFrame,
            GPRInfo::regT2);

[Michael Saboff]

Created attachment 263602 [details]
Patch for Landing

[Michael Saboff]

Committed r191360: <http://trac.webkit.org/changeset/191360>