Bug 110412 – Building for WinCE (cmake) x86 targets fail

• Component: Platform
• Status: RESOLVED FIXED

[Mark Salisbury]

Source/cmake/OptionsWinCE.cmake makes an assumption that we will always build for ARM.

[Mark Salisbury]

Also, some cleanup - the WINCEBASIC define is no longer in use anywhere.  It was once used in JavaScriptCore\wtf\Assertions.cpp, but was removed in http://trac.webkit.org/changeset/68511

[Mark Salisbury]

Created attachment 189429 [details]
Proposed fix

[Laszlo Gombos]

Would it be possible/better to remove this from the WinCE build system and let Platform.h (http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Platform.h) define these ? Is there something missing from Platform.h ?

[Mark Salisbury]

Good question.  The platform SDK should define ARM, _ARM_, and the ARM architecture version.  (I checked and mine defines ARM and _ARM_, but not the version, which is 4I... I can address that).

Since a platform SDK targeting ARM defines ARM, there should be no need to explicitly define it in OptionsWinCE.

I'll do a test build (for ARM) showing that I can remove the WTF_CPU_ARM_TRADITIONAL and then upload a new patch.

I'd like to see a comment from Patrick since I think he created OptionsWinCE.cmake in the first place.

As things stand today, I can't build for x86 since OptionsWinCE assumes we will always target ARM.

[Mark Salisbury]

Created attachment 189559 [details]
Proposed fix

Unfortunately, none of the ARM architecture defines besides just "_ARM_" and "ARM" are set (this is either a problem with my platform SDK or with CMake), so it is necessary to define either WTF_CPU_ARM_THUMB2 or WTF_CPU_ARM_TRADITIONAL in OptionsWinCE.cmake when building for ARM.  I'm not the only one with this issue (as WTF_CPU_ARM_TRADITIONAL is defined now in OptionsWinCE.cmake).

With this patch, you can build for x86 and you can build for ARM.  If you choose, when you run CMake you can specify what your ARM architecture version is and if you have THUMB2 support.  I tried both of these, since I'm actually targeting a Cortex A8 CPU (ARM architecture v7).  When I did this I discovered some other issues that will need to be addressed later to enable ARM v7 architecture on WinCE.  If you don't specify the ARM architecture version or THUMB2 support, then you get WTF_CPU_ARM_TRADITIONAL defined (same as we have now) which does build successfully.

[Patrick R. Gansterer]

Comment on attachment 189559 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=189559&action=review

> ChangeLog:3
> +        [WinCE] Don't add definition for WTF_CPU_ARM_TRADITIONAL if we're not

subject should be only one line long

> Source/cmake/OptionsWinCE.cmake:17
> +        add_definitions(-DWTF_CPU_ARM_TRADITIONAL)

i don't like to see this in tis file.
What about changeing the Platform.h to make define ARM_TRADITONAL, when no ARM_THUMB2 was detected?

[Mark Salisbury]

Created attachment 189773 [details]
Proposed fix

Thanks for your review.

I simplified the description to one line: "Fix for building for x86 targets"

I think we don't want to change behavior in Platform.h.  I like the idea that if there aren't enough hints given from the compile command line about what your machine architecture is that you get an error that you need to deal with.

It's easy to focus on what this patch doesn't do, but here's what it does do:
1)  Addresses the bug I've filed (that you can't build the WinCE platform for x86)
2)  If you are building for ARM, if you don't do anything different current behavior will continue (WTF_ARM_TRADITIONAL will be defined by OptionsWinCE.cmake)
3)  If you are building for ARM, you have the option to specify your ARM architecture version and if you have THUMB2 support.

[Mark Salisbury]

Created attachment 189775 [details]
Proposed fix

Oops.  Forgot to regenerate patch after making change to ChangeLog.

[Mark Salisbury]

It's been a few days since I've heard anything.  Could someone take a minute to review this?  Thanks in advance.

[Patrick R. Gansterer]

I removed the WTF_CPU_ARM_TRADITIONAL define. This should fix the problem.

[Patrick R. Gansterer]

Please feel free to reopen it if you still have problems.

[Gyuyoung Kim]

Comment on attachment 189775 [details]
Proposed fix

Clearing r?, cq? because this patch is resolved fixed.