Bug 128226

Summary: The stack limit computation does not work for Windows
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, cmarcelo, commit-queue, fpizlo, ggaren, mhahnenberg, msaboff, oliver, roger_fong
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 127940    
Attachments:
Description Flags
fixed StackBound::recursionLimit() to handle underflows.
none
I fixed the patch, but apparently uploaded the wrong one. Round 2.
none
Trying again. none

Mark Lam
Reported 2014-02-04 18:16:14 PST
The current stack limit computation assumes that the stack is allocated in high memory. On Windows, the stack seems to be allocated very near address 0, and is smaller in size than our Options::maxPerThreadStackUsage(). This combination triggers a bug where StackBounds::recursionLimit() underflows and computes a limit address that is ridiculously high in memory. The net effect is that the Windows port thinks its out of stack space all the time. Fix coming.
Attachments
fixed StackBound::recursionLimit() to handle underflows. (5.76 KB, patch)
2014-02-04 18:48 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
I fixed the patch, but apparently uploaded the wrong one. Round 2. (5.76 KB, patch)
2014-02-04 19:28 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
Trying again. (5.77 KB, patch)
2014-02-04 19:33 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2014-02-04 18:48:55 PST
Created attachment 223195 [details] fixed StackBound::recursionLimit() to handle underflows.
Oliver Hunt
Comment 2 2014-02-04 18:51:39 PST
Comment on attachment 223195 [details] fixed StackBound::recursionLimit() to handle underflows. View in context: https://bugs.webkit.org/attachment.cgi?id=223195&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:156 > + CLoopRegister() { i = static_cast<intptr_t>(0xbadbeef0baddbeef); } tasty, but won't this break in 32bit builds?
Mark Lam
Comment 3 2014-02-04 18:52:46 PST
Comment on attachment 223195 [details] fixed StackBound::recursionLimit() to handle underflows. View in context: https://bugs.webkit.org/attachment.cgi?id=223195&action=review >> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:156 >> + CLoopRegister() { i = static_cast<intptr_t>(0xbadbeef0baddbeef); } > > tasty, but won't this break in 32bit builds? Nope. That's what the static_cast<intptr_t> takes care of for me. I verified that the 32-bit build is happy.
Mark Lam
Comment 4 2014-02-04 19:28:50 PST
Created attachment 223198 [details] I fixed the patch, but apparently uploaded the wrong one. Round 2.
Mark Lam
Comment 5 2014-02-04 19:30:05 PST
Comment on attachment 223198 [details] I fixed the patch, but apparently uploaded the wrong one. Round 2. A bad patch again. Redoing.
Mark Lam
Comment 6 2014-02-04 19:33:21 PST
Created attachment 223200 [details] Trying again.
Mark Lam
Comment 7 2014-02-04 19:35:05 PST
Patch 3 has passed the layout tests with a debug build.
Geoffrey Garen
Comment 8 2014-02-04 20:59:33 PST
Comment on attachment 223200 [details] Trying again. r=me
WebKit Commit Bot
Comment 9 2014-02-04 21:27:32 PST
Comment on attachment 223200 [details] Trying again. Clearing flags on attachment: 223200 Committed r163428: <http://trac.webkit.org/changeset/163428>
WebKit Commit Bot
Comment 10 2014-02-04 21:27:34 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.