WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
116661
[WIN] Implement correct detection of stack size
https://bugs.webkit.org/show_bug.cgi?id=116661
Summary
[WIN] Implement correct detection of stack size
Patrick R. Gansterer
Reported
2013-05-23 05:42:23 PDT
[WIN] Implement correct detection of stack size
Attachments
Patch
(6.60 KB, patch)
2013-05-23 05:45 PDT
,
Patrick R. Gansterer
no flags
Details
Formatted Diff
Diff
patch to roll Patrick's patch back in
(6.83 KB, patch)
2013-06-20 16:29 PDT
,
Mark Lam
bfulgham
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Patrick R. Gansterer
Comment 1
2013-05-23 05:45:19 PDT
Created
attachment 202668
[details]
Patch
WebKit Commit Bot
Comment 2
2013-05-23 11:50:41 PDT
Comment on
attachment 202668
[details]
Patch Clearing flags on attachment: 202668 Committed
r150600
: <
http://trac.webkit.org/changeset/150600
>
WebKit Commit Bot
Comment 3
2013-05-23 11:50:43 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 4
2013-05-23 19:31:51 PDT
The patch was rolled out in
http://trac.webkit.org/changeset/150621
because it broke Windows tests.
Patrick R. Gansterer
Comment 5
2013-05-23 22:11:16 PDT
(In reply to
comment #4
)
> The patch was rolled out in
http://trac.webkit.org/changeset/150621
because it broke Windows tests.
r150621
says "as it breaks the VS2010 builds" and nothing about broken tests. a) I built the patch with VS2010 and have no problem and b) I don't see the test failure at the bots
Brent Fulgham
Comment 6
2013-05-23 22:38:59 PDT
This change caused reproducible run-time crashes on all of our VS2010 build bots. I was mistaken in my comment that it caused a build failure; this was a run-time failure. For some reason, VS2005 builds seem to work properly with this patch. Perhaps something in the compiler changed between releases. Specific test failures were in the JavaScriptCore test suite.
Patrick R. Gansterer
Comment 7
2013-05-24 06:20:17 PDT
(In reply to
comment #6
)
> This change caused reproducible run-time crashes on all of our VS2010 build bots. I was mistaken in my comment that it caused a build failure; this was a run-time failure.
Which VS2010 bot? Where are they??? Since when is the VS2010 build "official"?
> For some reason, VS2005 builds seem to work properly with this patch. Perhaps something in the compiler changed between releases. > > Specific test failures were in the JavaScriptCore test suite.
Please show me a link the failure or tell me which test makes problem. What is the _exact_ platform. BTW: Please add a comment to the bug when rolling out. You rolled out _two_ patches and did not add any comment. Also it's hard to fix it when a) ChangeLog is wrong and there are no information about what's the exact problem. :-/
Brent Fulgham
Comment 8
2013-06-18 18:06:54 PDT
CC'ing Mark Lam for JSCore review.
Mark Lam
Comment 9
2013-06-19 01:27:17 PDT
Comment on
attachment 202668
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=202668&action=review
> Source/WTF/wtf/StackBounds.cpp:204 > + m_bound = static_cast<char*>(m_origin) - (reservedMemory.RegionSize - guardPage.RegionSize + committedMemory.RegionSize) + pageSize;
Are you sure that this formula is correct? I applied this patch in a local build and dumped all the BaseAddreses and RegionSizes. It looks like the 3 regions (uncommitted, guard page, and committed) are mutually exclusive. Note: according to the data I got from applying your patch, the "reservedMemory" region would be more accurately named "reservedButUncommittedMemory". I'll just use "uncommittedMemory" since the guardPage and committed regions are also "reserved". The size of the reserved address range should be (uncommitedMemory.RegionSize + guardPage.RegionSize + commitedMemory.RegionSize). The useable stack address range should therefore be (uncommittedMemory.RegionSize + committedMemory.RegionSize). This is in accordance with the specs at
http://msdn.microsoft.com/en-us/library/ms686774%28VS.85%29.aspx
which states that "The system commits additional pages from the reserved stack memory as they are needed, until either the stack reaches the reserved size minus one page (which is used as a guard page to prevent stack overflow) …". Hence, m_bound should be (with reservedMemory renamed to uncommittedMemory): m_bound = static_cast<char*>(m_origin) - (uncommittedMemory.RegionSize + committedMemory.RegionSize); Can you please check the address ranges again and confirm if this is the case?
Mark Lam
Comment 10
2013-06-19 01:41:03 PDT
(In reply to
comment #7
)
> BTW: Please add a comment to the bug when rolling out. You rolled out _two_ patches and did not add any comment. Also it's hard to fix it when a) ChangeLog is wrong and there are no information about what's the exact problem. :-/
BTW, regarding the test that failed, I'm seeing failures when running Tools/Scripts/run-javascriptcore-tests. I know of at least 2 issues when running these tests: 1. There's an assertion failure in the StackPolicy in Interpreter.cpp. The StackPolicy expects that the total stack size must be greater than 256k. In my test run on Windows, the total reserved space of the stack region is 256k. Minus the 4k guard page, it's certainly less than 256k. This 256k value in the StackPolicy was picked based on some heuristics. We'll need to rethink how we want to handle this in light of Windows having a stack just under that. Not sure if this is the OS default or something we configured. 2. Once I reduced the StackPolicy requirement in my local Windows build, I saw another assertion failure in the testapi test (which is part of run-javascriptcore-tests). I have yet to isolate the root cause of this assertion failure. Still investigating.
Mark Lam
Comment 11
2013-06-19 10:31:50 PDT
I've filed
https://bugs.webkit.org/show_bug.cgi?id=117801
to deal with the assertion failure issues, and blocked this bug on that one. Apart from that, I still think that there is a bug in this patch as indicated in
comment #9
.
Mark Lam
Comment 12
2013-06-20 16:29:09 PDT
Created
attachment 205128
[details]
patch to roll Patrick's patch back in
Brent Fulgham
Comment 13
2013-06-20 16:34:20 PDT
Comment on
attachment 205128
[details]
patch to roll Patrick's patch back in r=me
Mark Lam
Comment 14
2013-06-20 16:37:40 PDT
The patch has been rolled back in in
r151810
: <
http://trac.webkit.org/changeset/151810
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug