Bug 20746 – Port WebKit to Qt on Windows CE

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Joerg Bornemann]

This bug report collects the patches for the Qt for Windows CE port of WebKit.

[Simon Hausmann]

Created attachment 23293 [details]
This patch adds the PLATFORM_WINCE macro

[Simon Hausmann]

Created attachment 23294 [details]
Main part of the Windows CE port

[Simon Hausmann]

Created attachment 23295 [details]
This patch from George Staikos implements currentThreadStackBase on CE

[Simon Hausmann]

Created attachment 23296 [details]
Change DateMath.cpp to not use errno with strtol

[Simon Hausmann]

Created attachment 23297 [details]
Fix compilation on Windows CE when using some C string functions

[Mark Rowe (bdash)]

Comment on attachment 23293 [details]
This patch adds the PLATFORM_WINCE macro

I think that using WIN_CE for the macro name may be slightly clearer, though WINCE may more accurately reflect the emotions associated with Windows CE.

[Mark Rowe (bdash)]

Simon, is there some reason that you did not change the macro to WIN_CE like I suggested?

[Simon Hausmann]

(In reply to comment #7)
> Simon, is there some reason that you did not change the macro to WIN_CE like I
> suggested?

I misunderstood your comment halfway joking, sorry. We can change it to WIN_CE, but the main reason why we called it WINCE is because it seemed the most consistent option.

All the other platform macros use two underscores in their name, only WIN_OS has three.

[Mark Rowe (bdash)]

(In reply to comment #8)
> (In reply to comment #7)
> > Simon, is there some reason that you did not change the macro to WIN_CE like I
> > suggested?
> 
> I misunderstood your comment halfway joking, sorry. We can change it to WIN_CE,
> but the main reason why we called it WINCE is because it seemed the most
> consistent option.
> 
> All the other platform macros use two underscores in their name, only WIN_OS
> has three.

As far as I'm aware, all of our other platform macros are a single word so there would be nowhere to put an underscore.  On the other hand, Windows CE is two words.

[Simon Hausmann]

Created attachment 23338 [details]
Make the Qt port of WebKit compile on Windows CE (updated for WINCE -> WIN_CE)

[Simon Hausmann]

Created attachment 23339 [details]
Implemented currentThreadStackBase() on Windows CE. (updated WINCE -> WIN_CE)

[Simon Hausmann]

Created attachment 23340 [details]
Fix compilation on Windows CE when using some C string functions (updated for WINCE -> WIN_CE)

[Simon Hausmann]

Committed WINCE -> WIN_CE change in r36328 and updated all the patches that used the macro

[Simon Hausmann]

Comment on attachment 23293 [details]
This patch adds the PLATFORM_WINCE macro

Clearing review flag since it's been landed

[Eric Seidel (no email)]

Comment on attachment 23339 [details]
Implemented currentThreadStackBase() on Windows CE. (updated WINCE -> WIN_CE)

What's with the #if 1 ?

[Eric Seidel (no email)]

Comment on attachment 23296 [details]
Change DateMath.cpp to not use errno with strtol

Seems cleaner would be to just have a wrapper function which handled using errno or not and returned a bool.  We are generally against Macros in WebKit code (as they make things harder to read and debug).

Here would be an example:

static bool parseLong(const char* string, char*& stopPosition, long maxLength)
{
#if HAVE(ERRNO)
   strtol(string, stopPosition, maxLength);
   return (errno = 0);
#else
  // wince path
#endif
}

[Eric Seidel (no email)]

Comment on attachment 23340 [details]
Fix compilation on Windows CE when using some C string functions (updated for WINCE -> WIN_CE)

I'd like aroben or darin to look at this, as they're more versed on the status/purpose of StringExtras.  The change in general looks fine to me.

[Eric Seidel (no email)]

Comment on attachment 23338 [details]
Make the Qt port of WebKit compile on Windows CE (updated for WINCE -> WIN_CE)

In general this looks OK. I can't say I've given this the most thorough review.  The patch is a bit too big. :(

Tab:
+    time_t timet = mktime(&tmtime);

This feels strange, that generally the check is a compile check except for WIN_CE:
+#if COMPILER(MSVC7) || COMPILER(MINGW) || PLATFORM(WIN_CE)

Makes me wonder which set of checks is wrong...

Again, should the MSVC one be WIN_OS instead?
+#if COMPILER(MSVC) && !PLATFORM(WIN_CE)


It seems you do this in a couple places:

+#if PLATFORM(WIN_CE)
+        /* On Windows CE there's no abort() and on regular Windows
+           abort() exits with exit code 3. */
+        exit(3);
+#else
         abort();
+#endif

Maybe abort() should just be defined as an inline in some high-level header?

Again, this seems strange, but maybe this is the best check to go with?
+#if !COMPILER(MSVC7) && !PLATFORM(WIN_CE)

Seems this should be an #else clause on the above HAVE(ERRNO_H):
+#if PLATFORM(WIN_CE)
+#define NO_ERRNO
+#endif
or?  Are there platforms which have errno but not in ERRNO_H?

I'm not sure whether we'd generally try to convert such a file to WebKit style or not:
+++ b/JavaScriptCore/os-wince/ce_time.cpp
probably not.  The license on that file looks fine.

Redundant, since you included it in Assertions.h as well:
+
+#if PLATFORM(WIN_CE)
+#include <windows.h>
+#endif
+

Again, seems like the MSVC checks shoudl really be WIN_OS checks:
+#if COMPILER(MSVC) && !PLATFORM(WIN_CE)
 #ifndef WINVER

Is there any cleaner way to do this?  In Chromium, we avoid including windows.h in any header.  I assume you'd probably want to do the same.  I think we avoid including it to avoid compile slowness or object file bloat.  But maybe I'm misinformed.

If you defined this in FastMalloc.h instead of FastMalloc.cpp, all .cpp files in WebCore would get access to your abort() function:
+#if PLATFORM(WIN_CE)
+/* On Windows CE there's no abort() and on regular Windows
+   abort() exits with exit code 3. */
+static void abort()
+{
+    exit(3);
+}
+#endif

I'm somewhat confused by this statement:
+/* PLATFORM(WIN_CE) && PLATFORM(QT)
+   We can not determine the endianess at compile time. For
+   Qt for Windows CE the endianess is specified in the
+   device specific makespec
+*/

Why did you add:
+#define HAVE_SYS_TYPES_H 1
in 3 places in Platform.h?

Another place that I wonder why 2 of these are compiler checks and one is an OS check:
+#if COMPILER(MINGW) || COMPILER(MSVC7) || PLATFORM(WIN_CE)

I guess I'll r- this, requesting a second revision (possibly broken into smaller pieces).  I guess an alternative would be to find someone more windows-savvy who'd like to review the whole thing for you. :)

[Joerg Bornemann]

Created attachment 23466 [details]
Implemented currentThreadStackBase() on Windows CE. (updated)

Removed the unused code block (inside the #if 1 ... #else ... #endif).

[Joerg Bornemann]

Created attachment 23468 [details]
Change DateMath.cpp to not use errno with strtol (updated)

Updated patch 23296 to use a function instead of a macro to wrap strtol.

[Joerg Bornemann]

Comment on attachment 23468 [details]
Change DateMath.cpp to not use errno with strtol (updated)

ooops wrong patch

[Joerg Bornemann]

Created attachment 23469 [details]
Change DateMath.cpp to not use errno with strtol (updated)

Updated patch 23296 to use a function instead of a macro to wrap strtol.

[Joerg Bornemann]

(In reply to comment #18)

> Tab:
> +    time_t timet = mktime(&tmtime);

Will fix this.
 
> This feels strange, that generally the check is a compile check except for
> WIN_CE:
> +#if COMPILER(MSVC7) || COMPILER(MINGW) || PLATFORM(WIN_CE)
> 
> Makes me wonder which set of checks is wrong...

We've discussed this check too. Actually PLATFORM(WIN_CE) is both, a platform check and a compiler check. For Windows CE MSVC uses a different compiler (and different includes) than for Windows 32. We could add a MSVC_CE compiler define which is always defined if PLATFORM(WINCE) and COMPILER(MSVC). But since there's only one serious Windows CE compiler available, we didn't bother...

> It seems you do this in a couple places:
> 
> +#if PLATFORM(WIN_CE)
> +        /* On Windows CE there's no abort() and on regular Windows
> +           abort() exits with exit code 3. */
> +        exit(3);
> +#else
>          abort();
> +#endif
> 
> Maybe abort() should just be defined as an inline in some high-level header?

There are only two files where we do this:
JavaScriptCore/kjs/collector.cpp and 
JavaScriptCore/wtf/FastMalloc.cpp
Do we really want to infest the global namespace with an abort definition?

> Seems this should be an #else clause on the above HAVE(ERRNO_H):
> +#if PLATFORM(WIN_CE)
> +#define NO_ERRNO
> +#endif
> or?  Are there platforms which have errno but not in ERRNO_H?

Hm...you're right, it seems unlikely that there are platforms that define errno but have no errno.h. Will fix this.
 
> Redundant, since you included it in Assertions.h as well:
> +
> +#if PLATFORM(WIN_CE)
> +#include <windows.h>
> +#endif
> +

Removed.

> Again, seems like the MSVC checks shoudl really be WIN_OS checks:
> +#if COMPILER(MSVC) && !PLATFORM(WIN_CE)
>  #ifndef WINVER

No, because WIN_OS is also defined for MinGW.
 
> Is there any cleaner way to do this?  In Chromium, we avoid including windows.h
> in any header.  I assume you'd probably want to do the same.  I think we avoid
> including it to avoid compile slowness or object file bloat.  But maybe I'm
> misinformed.

We had a little discussion on webkit-dev about this. The main problem is, that WebKit defines the ASSERT macro. The Windows headers for Win CE define that too and are broken in that sense that we *must* first include <windows.h> then undefine ASSERT and define our own ASSERT macro.
 
> I'm somewhat confused by this statement:
> +/* PLATFORM(WIN_CE) && PLATFORM(QT)
> +   We can not determine the endianess at compile time. For
> +   Qt for Windows CE the endianess is specified in the
> +   device specific makespec
> +*/

For gcc on ARM you have several defines from which you can determine the endianess of the platform. For MSVC for Windows CE this isn't possible because the defines are missing. Qt has defines of the endianess in the mkspecs for the specific platform.
 
> Why did you add:
> +#define HAVE_SYS_TYPES_H 1
> in 3 places in Platform.h?

To mark all the platforms that have sys/types.h which are DARWIN, Win32 and the default case.

> I guess I'll r- this, requesting a second revision (possibly broken into
> smaller pieces).  I guess an alternative would be to find someone more
> windows-savvy who'd like to review the whole thing for you. :)
 
I hope that my comments explain a little bit. :)

[Joerg Bornemann]

Created attachment 23470 [details]
Make the Qt port of WebKit compile on Windows CE (updated)

[Eric Seidel (no email)]

Comment on attachment 23466 [details]
Implemented currentThreadStackBase() on Windows CE. (updated)

Please use WebKit style for your changes.
http://webkit.org/coding/contributing.html

Also, I think that the "abort()" definition should go in config.h or some header which config.h (or whatever pre-compiled header your WinCE build uses).   It's silly to need to have #ifdefs around every abort() usage.  Another way would be to change all abort usages to some webkit-specific function (like ABORT()) or similar, but I think it's better to just define abort() for WinCE.

[Darin Adler]

Comment on attachment 23340 [details]
Fix compilation on Windows CE when using some C string functions (updated for WINCE -> WIN_CE)

We prefer to use inline functions instead of macros for cases like this. Look at the inline function for strncasecmp right below for an example of what I'm talking about.

(If we did use macros, we'd want to use macros with argument names rather than just doing the name of the function alone, but that's a moot point -- lets use inline functions.)

I'm going to say review-, but I'd love to see a version of this that used inline functions instead.

[Eric Seidel (no email)]

Comment on attachment 23469 [details]
Change DateMath.cpp to not use errno with strtol (updated)

This is nice.  2 comments though.

This will crash if endPtr is null:
+    return string != *endPtr && retVal != LONG_MIN && retVal != LONG_MAX;

Maybe strtol would also crash in that situation, not sure?

Also, since this is our own function:
+static bool parseLong(long& retVal, const char* string, char** endPtr, int base)

I think it's clearer (less typing at least), to use:
+static bool parseLong(long& retVal, const char* string, char*& endPtr, int base)

Then one doesn't need to add & to every callsite.

We also could make base =10 a default paramteter to further clean up the callsites.

I think this patch is really OK as is, but given that we're in this code, I feel like we should clean it up a bit more.  I guess I could do that and post a response patch for you.  I'll do that.

[Eric Seidel (no email)]

Comment on attachment 23469 [details]
Change DateMath.cpp to not use errno with strtol (updated)

Turns out that Windows now has some additional time code, so maybe WinCE can use that, instead of it's own custom stuff.

[Joerg Bornemann]

Created attachment 23698 [details]
Fix compilation on Windows CE when using some C string functions (uses inline functions)

As Darin suggested in comment #26, I've replaced the macros with inline functions.

[Joerg Bornemann]

(In reply to comment #28)
> (From update of attachment 23469 [details] [edit])
> Turns out that Windows now has some additional time code, so maybe WinCE can
> use that, instead of it's own custom stuff.

Hmm...interesting! I will have a look at the new Windows time stuff and see if its usable for Windows CE. :)

[Darin Adler]

Comment on attachment 23698 [details]
Fix compilation on Windows CE when using some C string functions (uses inline functions)

r=me

[Darin Adler]

Comment on attachment 23698 [details]
Fix compilation on Windows CE when using some C string functions (uses inline functions)

Clearing review flag since this was landed: http://trac.webkit.org/changeset/37604

[Joerg Bornemann]

Created attachment 25262 [details]
Implemented currentThreadStackBase() on Windows CE. (updated)

This patch update contains beautification of the last patch respecting the WebKit coding style guide. It doesn't contain the #ifdef for abort anymore. It must be declared globally, because it is already defined (but not declared) in the Windows CE C runtime.

[Darin Adler]

Comment on attachment 25262 [details]
Implemented currentThreadStackBase() on Windows CE. (updated)

> +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?

[Joerg Bornemann]

(In reply to comment #34)

Thanks for your feedback. Looks like the time is right for rewriting the whole function.

[Simon Hausmann]

Comment on attachment 23470 [details]
Make the Qt port of WebKit compile on Windows CE (updated)

Clearing the review flag. The patch doesn't apply against trunk anymore due to many renamed files. I'm splitting the patch up into smaller pieces and review them individually. Let's see what remains after that :)

[Simon Hausmann]

Created attachment 25571 [details]
Implemented currentThreadStackBase() on Windows CE (slight coding style fixes)

This updated patch addresses some of the coding style issues.

[George Staikos]

We have a slightly different implementation since this one.  I would prefer if we could first get back to PLATFORM(WINCE) as discussed months ago, and start aligning with our native CE port approach, and then we can merge our updates for this code piecewise.  It's our intentions to start that merging soon.

Regarding the algorithm, this is the best we can find so far.

[Simon Hausmann]

AFAICS all necessary patches are in the trunk now, closing this bug.