Bug 26611 - Implement currentThreadStackBase on WinCE
: Implement currentThreadStackBase on WinCE
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: PC Other
: P2 Normal
Assigned To:
:
:
:
: 23154
  Show dependency treegraph
 
Reported: 2009-06-22 10:56 PST by
Modified: 2009-06-23 08:47 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-22 10:56:21 PST
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 From 2009-06-22 11:08:02 PST -------
Created an attachment (id=31657) [details]
currentThreadStackBase for WinCE

This patch is from Yong Li.
------- Comment #2 From 2009-06-22 11:24:20 PST -------
(From update of attachment 31657 [details])
> +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 From 2009-06-22 11:33:03 PST -------
(From update of attachment 31657 [details])
> +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 From 2009-06-22 11:36:38 PST -------
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 From 2009-06-22 15:43:37 PST -------
Created an attachment (id=31682) [details]
updated patch

This should fix the above issues
------- Comment #6 From 2009-06-23 07:43:15 PST -------
(From update of attachment 31682 [details])
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 From 2009-06-23 07:44:25 PST -------
Also, Yong wrote some of this right?  The ChangeLog should include all authors.
------- Comment #8 From 2009-06-23 07:59:17 PST -------
Created an attachment (id=31720) [details]
third patch
------- Comment #9 From 2009-06-23 08:47:06 PST -------
Landed with r44993.