Bug 26611 – Implement currentThreadStackBase on WinCE

• Component: Platform
• Status: RESOLVED FIXED

[Joe Mason]

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.

[Joe Mason]

Created attachment 31657 [details]
currentThreadStackBase for WinCE

This patch is from Yong Li.

[Adam Treat]

Comment on attachment 31657 [details]
currentThreadStackBase for WinCE

> [phone]  Yong Li  <[e-mail]>
> +
> +        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 on attachment 31657 [details]
currentThreadStackBase for WinCE

> [phone]  Yong Li  <[e-mail]>
> +
> +        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]

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]

Created attachment 31682 [details]
updated patch

This should fix the above issues

[Adam Treat]

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]

Also, Yong wrote some of this right?  The ChangeLog should include all authors.

[Joe Mason]

Created attachment 31720 [details]
third patch

[Adam Treat]

Landed with r44993.