RESOLVED FIXED 26611
Implement currentThreadStackBase on WinCE 
https://bugs.webkit.org/show_bug.cgi?id=26611
Summary Implement currentThreadStackBase on WinCE
Joe Mason
Reported 2009-06-22 10:56:21 PDT
WinCE has no function to find the stack base, so we keep a global, g_stackBase, which must be set to the address of a local variable by the caller before calling any WebKit function that invokes JSC. currentThreadStackBase simply returns this value if it is set. If g_stackBase is not set, as a workaround currentThreadStackBase finds the top of the stack (address of a local variable), and then goes through all writeable pages reachable from this address. This actually returns an area much bigger than the actual stack range, so some dead objects can't be collected, but it guarantees live objects aren't collected prematurely.
Attachments
currentThreadStackBase for WinCE (2.48 KB, patch)
2009-06-22 11:08 PDT, Joe Mason 		manyoso: review-
DetailsFormatted DiffDiff
updated patch (3.66 KB, patch)
2009-06-22 15:43 PDT, Joe Mason 		manyoso: review-
DetailsFormatted DiffDiff
third patch (3.87 KB, patch)
2009-06-23 07:59 PDT, Joe Mason 		manyoso: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Joe Mason
Comment 1 2009-06-22 11:08:02 PDT
Created attachment 31657 [details] currentThreadStackBase for WinCE This patch is from Yong Li.
Adam Treat
Comment 2 2009-06-22 11:24:20 PDT
Comment on attachment 31657 [details] currentThreadStackBase for WinCE > +2009-06-22 Yong Li <yong.li@torchmobile.com> > + > + Reviewed by NOBODY (OOPS!). > + > + https://bugs.webkit.org/show_bug.cgi?id=26611 > + Implement currentThreadStackBase on WINCE You should include the full description of the change here. As well as document each new function you are adding where it is non-obvious. Good to be concise, but not at the expense of true explanation. > +inline bool isPageWritable(void* p) Please do not abbreviate the names of variables. Spell out the variable using a good descriptive name. Err on code readability and maintenance instead of less typing. > + void* sp = (void*)(&lower); > + register char* curPage = (char*)((DWORD)sp & ~(pageSize - 1)); Same as above.
Adam Treat
Comment 3 2009-06-22 11:33:03 PDT
Comment on attachment 31657 [details] currentThreadStackBase for WinCE > +2009-06-22 Yong Li <yong.li@torchmobile.com> > + > + Reviewed by NOBODY (OOPS!). > + > + https://bugs.webkit.org/show_bug.cgi?id=26611 > + Implement currentThreadStackBase on WINCE You should include the full description of the change here. As well as document each new function you are adding where it is non-obvious. Good to be concise, but not at the expense of true explanation. > +inline bool isPageWritable(void* p) Please do not abbreviate the names of variables. Spell out the variable using a good descriptive name. Err on code readability and maintenance instead of less typing. > + void* sp = (void*)(&lower); > + register char* curPage = (char*)((DWORD)sp & ~(pageSize - 1)); Same as above.
Joe Mason
Comment 4 2009-06-22 11:36:38 PDT
From Adam: Also, the 'for (;;)' looks dangerous if 'isPageWritable' never returns false. We can fix this by checking that curPage > 0 when scanning downwards, or < some max bound when scanning upwards. Yong suggests 0x8000000.
Joe Mason
Comment 5 2009-06-22 15:43:37 PDT
Created attachment 31682 [details] updated patch This should fix the above issues
Adam Treat
Comment 6 2009-06-23 07:43:15 PDT
Comment on attachment 31682 [details] updated patch Much better style, but still a few nits: > +inline bool isPageWritable(void* page) > +{ > + MEMORY_BASIC_INFORMATION buf; > + DWORD result = VirtualQuery(page, &buf, sizeof(buf)); s/buf/buffer/ > + if (!pageSize) { > + SYSTEM_INFO sysInfo; s/sysInfo/systemInfo/ > + register char* curPage = (char*)((DWORD)thisFrame & ~(pageSize - 1)); s/curPage/currentPage/
Adam Treat
Comment 7 2009-06-23 07:44:25 PDT
Also, Yong wrote some of this right? The ChangeLog should include all authors.
Joe Mason
Comment 8 2009-06-23 07:59:17 PDT
Created attachment 31720 [details] third patch
Adam Treat
Comment 9 2009-06-23 08:47:06 PDT
Landed with r44993.
Note You need to log in before you can comment on or make changes to this bug.