Bug 29715 - [All] RVCT compilation of WebKit fails due to ARM/Thumb defines in Platform.h
Summary: [All] RVCT compilation of WebKit fails due to ARM/Thumb defines in Platform.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 29725 (view as bug list)
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-09-24 08:31 PDT by George Wright
Modified: 2009-10-01 00:14 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch (1.56 KB, patch)
2009-09-24 15:42 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
2nd patch, independent of 1st. (1.91 KB, patch)
2009-09-24 16:32 PDT, Laszlo Gombos
barraclough: review-
Details | Formatted Diff | Diff
Check RVCT macros as well for ARM_THUMB2 JIT (2.30 KB, patch)
2009-09-25 03:16 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Check RVCT macros for ARM_THUMB2 platform (2.33 KB, patch)
2009-09-25 03:37 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Check RVCT macros for ARM_THUMB2 platform (v2) (4.29 KB, patch)
2009-09-25 10:49 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Check RVCT macros for ARM compilation (4.69 KB, patch)
2009-09-30 14:05 PDT, George Wright
staikos: review-
Details | Formatted Diff | Diff
RVCT macros fix - take 2 (4.56 KB, patch)
2009-09-30 14:16 PDT, George Wright
no flags Details | Formatted Diff | Diff
RVCT macros fix - take 3 (4.56 KB, patch)
2009-09-30 14:19 PDT, George Wright
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Wright 2009-09-24 08:31:31 PDT
The patch landed from bug #29122 seems to break ARM compiles when using RVCT. The patch uses __thumb2__ and thumb2 compile predefined macros to check for whether Thumb2 support is available, but RVCT does not set these. See http://www.keil.com/support/man/docs/armccref/armccref_BABJIDGC.htm.

I believe the solution would be to add checks based on __TARGET_ARCH_ARM and __TARGET_ARCH_THUMB to Platform.h
Comment 1 George Staikos 2009-09-24 09:53:41 PDT
Patch?
Comment 2 Laszlo Gombos 2009-09-24 15:42:37 PDT
Created attachment 40088 [details]
proposed patch

The ARM HW detection rules seems to be specific (to GCC) and does not work for Symbian. Add detection rules for the Symbain toolchain.
Comment 3 Laszlo Gombos 2009-09-24 15:44:49 PDT
*** Bug 29725 has been marked as a duplicate of this bug. ***
Comment 4 Laszlo Gombos 2009-09-24 15:46:25 PDT
(In reply to comment #2)
> Created an attachment (id=40088) [details]
> proposed patch
> 
> The ARM HW detection rules seems to be specific (to GCC) and does not work for
> Symbian. Add detection rules for the Symbain toolchain.

This patch solves the problem for Symbian but not for other platforms.
Comment 5 Laszlo Gombos 2009-09-24 16:32:38 PDT
Created attachment 40091 [details]
2nd patch, independent of 1st.

Only perform ARM arch sanity check if JIT is enabled; Geaorge if JIT is not enabled this patch should probably solve your problem. Can you check ?
Comment 6 Eric Seidel (no email) 2009-09-24 16:36:34 PDT
Comment on attachment 40091 [details]
2nd patch, independent of 1st.

The previous indent trick was easier to read.
Comment 7 Eric Seidel (no email) 2009-09-24 16:37:31 PDT
CCing reviewers who actually know something about ARM (I don't).
Comment 8 Gavin Barraclough 2009-09-24 18:03:29 PDT
Comment on attachment 40091 [details]
2nd patch, independent of 1st.

It doesn't make a lot of sense to me to make the presence of the traditional/thumb2 define dependent on the assembler, and this behaviour may be confusing to any developer not expecting this.  I'd suggest something like:

/* Defines two pseudo-platforms for ARM and Thumb-2 instruction set. */
#if !defined(WTF_PLATFORM_ARM_TRADITIONAL) && !defined(WTF_PLATFORM_ARM_THUMB2)
    #if defined(thumb2) || defined(__thumb2__)
    #define WTF_PLATFORM_ARM_THUMB2 1
    #else
    #define WTF_PLATFORM_ARM_TRADITIONAL 1
    #endif
#endif // !defined(WTF_PLATFORM_ARM_TRADITIONAL) && !defined(WTF_PLATFORM_ARM_THUMB2)
/* Sanity Check */
#if PLATFORM(ARM_TRADITIONAL) == PLATFORM(ARM_THUMB2)
    #error "Must define *either* PLATFORM(ARM_TRADITIONAL) *or* PLATFORM(ARM_THUMB2)."
#endif // PLATFORM(ARM_TRADITIONAL) == PLATFORM(ARM_THUMB2)
Comment 9 Janne Koskinen 2009-09-24 23:45:41 PDT
I don't see ARM7 check in the patch.
For reference this is how it was patched in QtWebkit to be able to compile again:
http://gitorious.org/qtwebkit/qtwebkit/commit/272cfd8765e96927ad9be950a0d4c32d0c8bd09a

I agree on the additional patch. What I don't like is the originating patch using name 'traditional' when referring to THUMB/ARM/ARMI ?
What about if I want compile for ARM instruction set on THUMB2 enabled HW ?
That would resolve to traditional assuming compiler used to not to pass __THUMB2__ flag. It should be more specific what 'traditional' is.
Comment 10 Gabor Loki 2009-09-25 00:23:13 PDT
(In reply to comment #9)
> http://gitorious.org/qtwebkit/qtwebkit/commit/272cfd8765e96927ad9be950a0d4c32d0c8bd09a

-# elif PLATFORM_ARM_ARCH(4)
+# elif PLATFORM_ARM_ARCH(4) || PLATFORM_ARM_ARCH(5)

The additional check with PLATFORM_ARM_ARCH(5) is unnecessary. See the definition of PLATFORM_ARM_ARCH.

> What about if I want compile for ARM instruction set on THUMB2 enabled HW ?
> That would resolve to traditional assuming compiler used to not to pass
> __THUMB2__ flag. It should be more specific what 'traditional' is.

Well, currently there is no way to use a different instruction set for JIT while the binary is used a different one. The thumb interworking with the current trampolines is not possible. So if you compile the binary in thumb2 mode, your JIT will be thumb2 as well (and vice versa).
Comment 11 Gabor Loki 2009-09-25 01:06:10 PDT
(In reply to comment #8)
...
>     #if defined(thumb2) || defined(__thumb2__)
>     #define WTF_PLATFORM_ARM_THUMB2 1
>     #else
>     #define WTF_PLATFORM_ARM_TRADITIONAL 1
>     #endif
...
> #if PLATFORM(ARM_TRADITIONAL) == PLATFORM(ARM_THUMB2)

Gavin, the binary operator '==' will fail if the complementer value of those macros are not defined.
Comment 12 Gabor Loki 2009-09-25 03:16:23 PDT
Created attachment 40103 [details]
Check RVCT macros as well for ARM_THUMB2 JIT

This patch adds an additional check for ARM_THUMB2 platform when the compiler is RVCT. The RVCT compiler does not defines 'thumb2' and/or '__thumb2__'. It only defines 'thumb' and '__thumb__' macros in Thumb2 mode.

In addition, the ARMv6T2 cpu have to be in ARMv6 group.
Comment 13 Gabor Loki 2009-09-25 03:22:37 PDT
(In reply to comment #9)
> http://gitorious.org/qtwebkit/qtwebkit/commit/272cfd8765e96927ad9be950a0d4c32d0c8bd09a

Just one more comment :)  I do not know if __MARM__ check is necessary. Because the RVCT compiler defines __arm__ macro.

Could someone confim that '__arm__' and/or 'arm' macros are missing on Symbian with RVCT?
Comment 14 Gabor Loki 2009-09-25 03:37:19 PDT
Created attachment 40104 [details]
Check RVCT macros for ARM_THUMB2 platform

Ops, I attached a wrong patch.

This patch should fix the original problem.
Comment 15 Laszlo Gombos 2009-09-25 05:06:45 PDT
(In reply to comment #13)
> (In reply to comment #9)
> > http://gitorious.org/qtwebkit/qtwebkit/commit/272cfd8765e96927ad9be950a0d4c32d0c8bd09a
> 
> Just one more comment :)  I do not know if __MARM__ check is necessary. Because
> the RVCT compiler defines __arm__ macro.
> 
> Could someone confim that '__arm__' and/or 'arm' macros are missing on Symbian
> with RVCT?

In my Symbian environment with RVCT the extra test for __MARM__ was not necessary.
Comment 16 Yong Li 2009-09-25 07:35:10 PDT
> -#  if defined(thumb2) || defined(__thumb2__)
> +#  if defined(thumb2) || defined(__thumb2__) || (defined(__TARGET_ARCH_THUMB) && __TARGET_ARCH_THUMB == 4)
> +       || ((defined(thumb) || defined(__thumb__)) && (defined(__ARM_ARCH_6T2__) || PLATFORM_ARM_ARCH(7)))
>  #    define WTF_PLATFORM_ARM_TRADITIONAL 0
>  #    define WTF_PLATFORM_ARM_THUMB2 1
>  #  elif PLATFORM_ARM_ARCH(4)
> -- 
> 1.6.0.4
> 

Could you please also check TARGET_ARCH_ARM? For the case TARGET_ARCH_ARM contains the version number but __ARM_ARCH_<n>__ is not defined.
Comment 17 Yong Li 2009-09-25 07:36:18 PDT
for example:

#define ARM_ARCH_VERSION 7

+#if !defined(ARM_ARCH_VERSION) && defined(__TARGET_ARCH_ARM)
+#define ARM_ARCH_VERSION __TARGET_ARCH_ARM
+#endif

#endif
Comment 18 WebKit Commit Bot 2009-09-25 09:10:58 PDT
Comment on attachment 40088 [details]
proposed patch

Clearing flags on attachment: 40088

Committed r48756: <http://trac.webkit.org/changeset/48756>
Comment 19 WebKit Commit Bot 2009-09-25 09:11:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Gabor Loki 2009-09-25 10:49:57 PDT
Created attachment 40123 [details]
Check RVCT macros for ARM_THUMB2 platform (v2)

I have rewritten the whole ARM/Thumb detection according to comment #8 and comment #17.

The detection of ARM_ARCH_VERSION is almost the same as it was, except __TARGET_ARM_ARCH macro from RVCT. A new macro - THUMB_ARCH_VERSION - has been introduced to detect Thumb-2 support. The detection mechanism is similar to the ARM one. And finally the ARM_THUMB2 platform will be set if the compiler sets '__thumb2__' macro or '__thumb__' one while THUMB_ARCH_VERSION is 4.
Comment 21 Gabor Loki 2009-09-25 10:55:28 PDT
Unfortunately, the commit bot closed this bug entry, but the original problem is still present.
Comment 22 Eric Seidel (no email) 2009-09-25 10:56:52 PDT
I think you're referring to bug 28230.  Sorry about that.
Comment 23 Gavin Barraclough 2009-09-25 13:23:44 PDT
(In reply to comment #11)
> (In reply to comment #8)
> ...
> >     #if defined(thumb2) || defined(__thumb2__)
> >     #define WTF_PLATFORM_ARM_THUMB2 1
> >     #else
> >     #define WTF_PLATFORM_ARM_TRADITIONAL 1
> >     #endif
> ...
> > #if PLATFORM(ARM_TRADITIONAL) == PLATFORM(ARM_THUMB2)
> 
> Gavin, the binary operator '==' will fail if the complementer value of those
> macros are not defined.

Loki,

No, the PLATFORM macro is defined as:

#define PLATFORM(WTF_FEATURE) (defined WTF_PLATFORM_##WTF_FEATURE  && WTF_PLATFORM_##WTF_FEATURE)

so the complement definitions are actually redundant.  Bear in mind, PLATFORM(X86) evaluates to false on ARM without WTF_PLATFORM_X86 being defined.  Do you have a compiler that this is causing a problem on? - works fine on OS X gcc.

cheers,
G.
Comment 24 Gabor Loki 2009-09-25 13:54:01 PDT
(In reply to comment #23)
> #define PLATFORM(WTF_FEATURE) (defined WTF_PLATFORM_##WTF_FEATURE  &&
> WTF_PLATFORM_##WTF_FEATURE)
> 
> so the complement definitions are actually redundant.  

Hmm. You are right. I probably made a typo when I tested that approach, and the compiler was complaining about missing side of binary operator '=='.

I have repeated the test, and it works fine.
Comment 25 George Wright 2009-09-30 14:05:37 PDT
Created attachment 40396 [details]
Check RVCT macros for ARM compilation

The previous patch which was r+d (and subsequently never committed) does not work on RVCT 3.x. This is an updated version of that patch which applies against a more up to date checkout of WebKit, and which allows for RVCT compilation.
Comment 26 George Staikos 2009-09-30 14:10:04 PDT
Comment on attachment 40396 [details]
Check RVCT macros for ARM compilation

Moving the || to the other side is against the coding guideline.  Fix that and I'll r+
Comment 27 George Staikos 2009-09-30 14:10:33 PDT
Comment on attachment 40123 [details]
Check RVCT macros for ARM_THUMB2 platform (v2)

Clear review flag as there are obvious issues here.
Comment 28 George Wright 2009-09-30 14:16:16 PDT
Created attachment 40397 [details]
RVCT macros fix - take 2

This revision moves the || operators to the left side as per the coding style guideline.
Comment 29 George Wright 2009-09-30 14:19:34 PDT
Created attachment 40398 [details]
RVCT macros fix - take 3

Missed one || move.
Comment 30 WebKit Commit Bot 2009-09-30 18:13:12 PDT
Comment on attachment 40398 [details]
RVCT macros fix - take 3

Clearing flags on attachment: 40398

Committed r48954: <http://trac.webkit.org/changeset/48954>
Comment 31 WebKit Commit Bot 2009-09-30 18:13:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Gabor Loki 2009-10-01 00:14:06 PDT
(In reply to comment #29)
> Created an attachment (id=40398) [details]

Thanks guys, I totally forgot about this patch.
By the way, it would be better with the previous ChangeLog entry - this patch does not define any pseudo-platform.