Bug 180366

Summary: We need to leave room on the top of the stack for the FTL TailCall slow path so it doesn't overwrite things we want to retrieve when doing a stack walk when throwing an exception
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
msaboff: review+
patch for landing none

Saam Barati
Reported 2017-12-04 11:08:30 PST
Otherwise, the slow path branch will overwrite things on the stack before calling into the slow path C call. Once we're in the C call, we realize we're actually going to throw an exception because we're trying to call something that isn't callable. Then we'll throw an exception, which does a stack walk. This stack walk will read the stack to recover the callee of inlined frames, which might have been overwritten with garbage already.
Attachments
patch (5.23 KB, patch)
2017-12-04 11:46 PST, Saam Barati
msaboff: review+
patch for landing (4.96 KB, patch)
2017-12-04 12:39 PST, Saam Barati
no flags
Saam Barati
Comment 1 2017-12-04 11:09:02 PST
I'm wondering if this is needed in the DFG as well. I suspect it is, but I have yet to prove it with a crashing test.
Saam Barati
Comment 2 2017-12-04 11:10:05 PST
Saam Barati
Comment 3 2017-12-04 11:15:57 PST
(In reply to Saam Barati from comment #1) > I'm wondering if this is needed in the DFG as well. I suspect it is, but I > have yet to prove it with a crashing test. The DFG already handles exactly this via Graph's m_parameterSlots which tracks the maximum outgoing call parameter count.
Saam Barati
Comment 4 2017-12-04 11:46:06 PST
Mark Lam
Comment 5 2017-12-04 12:00:57 PST
Comment on attachment 328368 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=328368&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6994 > + m_proc.requestCallArgAreaSizeInBytes( > + WTF::roundUpToMultipleOf(stackAlignmentBytes(), (CallFrame::headerSizeInRegisters + numArgs) * sizeof(EncodedJSValue))); AirCode::requestCallArgAreaSizeInBytes() already takes care of aligning to stackAlignmentBytes(). So, no need to do that here.
Saam Barati
Comment 6 2017-12-04 12:15:44 PST
Comment on attachment 328368 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=328368&action=review >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6994 >> + WTF::roundUpToMultipleOf(stackAlignmentBytes(), (CallFrame::headerSizeInRegisters + numArgs) * sizeof(EncodedJSValue))); > > AirCode::requestCallArgAreaSizeInBytes() already takes care of aligning to stackAlignmentBytes(). So, no need to do that here. I'm going to keep the rounding in to stay consistent w/ other places that do this in FTLLower
Michael Saboff
Comment 7 2017-12-04 12:33:26 PST
Comment on attachment 328368 [details] patch r=me
Saam Barati
Comment 8 2017-12-04 12:39:10 PST
Created attachment 328376 [details] patch for landing
WebKit Commit Bot
Comment 9 2017-12-04 14:00:27 PST
Comment on attachment 328376 [details] patch for landing Clearing flags on attachment: 328376 Committed r225492: <https://trac.webkit.org/changeset/225492>
WebKit Commit Bot
Comment 10 2017-12-04 14:00:29 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.