Bug 126009

Summary: CStack: Update the VMEntryScope's stack limit when the VM enters/exits ErrorMode
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mhahnenberg, msaboff, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 125928    
Attachments:
Description Flags
the patch.
none
the real patch.
none
patch 2. msaboff: review+

Mark Lam
Reported 2013-12-19 12:27:47 PST
See JSStack::enableErrorStackReserve() and JSStack::disableErrorStackReserve(). Just like this is needed for handling StackOverflowErrors when using the separate JSStack, we need to implement the equivalent for the C stack.
Attachments
the patch. (12.26 KB, patch)
2013-12-21 03:29 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
the real patch. (18.98 KB, patch)
2013-12-21 03:32 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 2. (19.43 KB, patch)
2013-12-21 03:46 PST, Mark Lam 		msaboff: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Geoffrey Garen
Comment 1 2013-12-19 15:02:02 PST
Can we just rely on handling errors inside the host zone? Do we need another concept?
Mark Lam
Comment 2 2013-12-19 15:04:59 PST
(In reply to comment #1) > Can we just rely on handling errors inside the host zone? Do we need another concept? We need a distinction because we need to allow the VM to use some additional stack space in order to do the work of throwing a StackOverflowError after we have reached the stack limit for normal execution. Otherwise, we’ll encounter a stack overflow while trying to throw the StackOverflowError.
Geoffrey Garen
Comment 3 2013-12-19 15:12:58 PST
(In reply to comment #2) > (In reply to comment #1) > > Can we just rely on handling errors inside the host zone? Do we need another concept? > > We need a distinction because we need to allow the VM to use some additional stack space in order to do the work of throwing a StackOverflowError after we have reached the stack limit for normal execution. Otherwise, we’ll encounter a stack overflow while trying to throw the StackOverflowError. Yes, that explains what we're trying to do. I have a suggestion for how: What you're calling "JSStack:: enableErrorStackReserve()" should just adjust the stack limit by half the size of the host zone.
Mark Lam
Comment 4 2013-12-19 15:16:18 PST
(In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > Can we just rely on handling errors inside the host zone? Do we need another concept? > > > > We need a distinction because we need to allow the VM to use some additional stack space in order to do the work of throwing a StackOverflowError after we have reached the stack limit for normal execution. Otherwise, we’ll encounter a stack overflow while trying to throw the StackOverflowError. > > Yes, that explains what we're trying to do. I have a suggestion for how: What you're calling "JSStack:: enableErrorStackReserve()" should just adjust the stack limit by half the size of the host zone. Yes, that is pretty much what I had in mind. I hadn’t thought of how much to adjust it by yet, but half the host zone sounds reasonable.
Mark Lam
Comment 5 2013-12-20 23:58:03 PST
The C stack limit tracking mechanism in the VMEntryScope class already has allowance for a smaller host zone when the Interpreter is in an ErrorMode. We just need to update the VMEntryScope's stack limit whenever the Interpreter enters and exits its ErrorMode.
Mark Lam
Comment 6 2013-12-21 03:29:56 PST
Created attachment 219845 [details] the patch.
Mark Lam
Comment 7 2013-12-21 03:31:11 PST
Comment on attachment 219845 [details] the patch. Wrong patch.
Mark Lam
Comment 8 2013-12-21 03:32:20 PST
Created attachment 219846 [details] the real patch.
Mark Lam
Comment 9 2013-12-21 03:45:01 PST
Comment on attachment 219846 [details] the real patch. View in context: https://bugs.webkit.org/attachment.cgi?id=219846&action=review > Source/JavaScriptCore/interpreter/JSStack.cpp:189 > +#else > + if (m_vm.entryScope) > + m_vm.entryScope->updateStackLimit(); > +#endif After thinking about this some more, the C stack update should be unconditional independent of whether the JS stack is on the C stack or its own separate stack. We may have detected a stack overflow due to the C stack reaching it's limit. Hence, the VM may need more C stack space to process the error even if the JS stack is on its own separate stack. I will upload an updated patch.
Mark Lam
Comment 10 2013-12-21 03:46:59 PST
Created attachment 219847 [details] patch 2.
Mark Lam
Comment 11 2013-12-21 11:22:31 PST
Landed in r160967 on the jsCStack branch: <http://trac.webkit.org/r160967>.
Mark Lam
Comment 12 2013-12-29 08:26:10 PST
Thanks for the review. Review status updated in r161117: <http://trac.webkit.org/r161117>.
Note You need to log in before you can comment on or make changes to this bug.