Summary: | REGRESSION (r191175): OSR Exit from an inlined tail callee trashes callee save registers | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | Keywords: | InRadar | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Michael Saboff
2015-10-19 11:55:31 PDT
Created attachment 263499 [details]
Reduced test case
Created attachment 263533 [details]
Patch
Comment on attachment 263533 [details]
Patch
r=me
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? (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); Created attachment 263602 [details]
Patch for Landing
Committed r191360: <http://trac.webkit.org/changeset/191360> |