Bug 26611 - Implement currentThreadStackBase on WinCE
: Implement currentThreadStackBase on WinCE
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: PC Other
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 23154
  Show dependency treegraph
 
Reported: 2009-06-22 10:56 PDT by Joe Mason
Modified: 2009-06-23 08:47 PDT (History)
2 users (show)

See Also:


Attachments
currentThreadStackBase for WinCE (2.48 KB, patch)
2009-06-22 11:08 PDT, Joe Mason
manyoso: review-
Details | Formatted Diff | Diff
updated patch (3.66 KB, patch)
2009-06-22 15:43 PDT, Joe Mason
manyoso: review-
Details | Formatted Diff | Diff
third patch (3.87 KB, patch)
2009-06-23 07:59 PDT, Joe Mason
manyoso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Mason 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.
Comment 1 Joe Mason 2009-06-22 11:08:02 PDT
Created attachment 31657 [details]
currentThreadStackBase for WinCE

This patch is from Yong Li.
Comment 2 Adam Treat 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.
Comment 3 Adam Treat 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.
Comment 4 Joe Mason 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.
Comment 5 Joe Mason 2009-06-22 15:43:37 PDT
Created attachment 31682 [details]
updated patch

This should fix the above issues
Comment 6 Adam Treat 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/
Comment 7 Adam Treat 2009-06-23 07:44:25 PDT
Also, Yong wrote some of this right?  The ChangeLog should include all authors.
Comment 8 Joe Mason 2009-06-23 07:59:17 PDT
Created attachment 31720 [details]
third patch
Comment 9 Adam Treat 2009-06-23 08:47:06 PDT
Landed with r44993.