[Webkit-unassigned] [Bug 20746] Port WebKit to Qt on Windows CE

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20746


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25262|review?(eric at webkit.org)    |review-
               Flag|                            |




------- Comment #34 from darin at apple.com  2008-11-19 09:23 PDT -------
(From update of attachment 25262)
> +static inline DWORD numberOfWritableBytes(void* p, void** base)

The caller passes 0, and there's only the one caller. So we should remove the
code that handles "base" in that case. We don't want to paste in a function
with dead code for a feature we aren't using.

> +    DWORD result;
> +    DWORD protect;

We normally define variables on the same line they're initialized on, not at
the top of the function.

> +    result = VirtualQuery(p, &buf, sizeof(buf));
> +    if (result != sizeof(buf))
> +        abort();

I don't think this particular assertion needs an explicit abort() rather than
just the normal ASSERT macro. In practice it's simply not going to happen.

> +    if (base != 0)
> +        *base = (void*)(buf.AllocationBase);
> +    protect = (buf.Protect & ~(PAGE_GUARD | PAGE_NOCACHE));
> +
> +    if (protect != PAGE_READWRITE && protect != PAGE_WRITECOPY &&
> +        protect != PAGE_EXECUTE_READWRITE && protect != PAGE_EXECUTE_WRITECOPY)
> +        return 0;
> +
> +    if (buf.State != MEM_COMMIT)
> +        return 0;

I don't think all these checks are helpful. You really need a comment
explaining why these are part of a useful way to detect the end of the stack.

A lot of the code in numberOfWritableBytes function simply doesn't apply to
pages in the stack. Most of those return 0 cases can't be hit at all, and if
they were hit you'd return the wrong value for the stack size.

> +static inline void* currentThreadStackBase_WIN_CE()

This above name does not follow WebKit naming conventions. There's no need for
the uppercase and the underscores.

> +    static DWORD pageSize = 0;
> +    if (!pageSize) {
> +        SYSTEM_INFO sysInfo;
> +        GetSystemInfo(&sysInfo);
> +        pageSize = sysInfo.dwPageSize;
> +    }

This could be made cleaner by putting the logic to get the page size into a
separate function so you wouldn't need the check for 0.

    static DWORD systemPageSize()
    {
        SYSTEM_INFO sysInfo;
        GetSystemInfo(&sysInfo);
        return sysInfo.dwPageSize;
    }

    static DWORD pageSize = systemPageSize();

> +    int dummy;
> +    void* sp = (void*)&dummy;

The typecast isn't helpful here. A void* can take an int*.

> +    char* trunc_sp = (char*)((DWORD)sp & ~(pageSize - 1));

This should use reinterpret_cast, not C-style casts. And trunc_sp does not
follow WebKit naming conventions.

> +    for (;;) {
> +        DWORD size = numberOfWritableBytes(trunc_sp, 0);
> +        if (size < pageSize)
> +            return trunc_sp + size;
> +        trunc_sp += pageSize;
> +    }

This algorithm is unnecessarily excessively inefficient. It calls VirtualQuery
on every page in the stack, even in the common case where the entire stack is
in a single virtual region.

This algorithm also has some wrong assumptions in it. The return statement adds
the size of the region to the passed in stack pointer value. But the size is
relative to the base of the region, not the passed-in pointer; you can't get a
sensible result by adding the size to a pointer that's in the middle of the
region. So this could easily return a value that's past the end of the region.
The reason that doesn't happen in practice is that region sizes are always
going to be multiples of page sizes anyway, so given the way the loop is
written the only value that numberOfWritableBytes can return that will result
in the loop ending is 0.

Since the assumption here is that the stack is composed of a number of virtual
regions, the code should get the region, then move to the end of that region,
and keep going as long as the regions are consistent with how the stack is
allocated. The first time we hit a bad region, then we've found the end of the
stack. There's no value to going a page at a time instead of a region at a time
and also no reason to round the stack pointer value to a page boundary. But
this also seems like a poor way to find the stack base. Is there really no
better way to do it? Is there no thread information block on WinCE?

> +#if PLATFORM(WIN_CE)
> +extern "C" void abort();
> +#endif

Is this declaration really necessary? For one thing, the value of the abort()
function call in the patch is extremely minimal; I'd suggest just removing it.
For another, abort() is a standard C function, normally defined in <stdlib.h>.
Can't we include that header instead?


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.