Bug 29715 – [All] RVCT compilation of WebKit fails due to ARM/Thumb defines in Platform.h

• Component: Platform
• Status: RESOLVED FIXED

[George Wright]

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

[George Staikos]

Patch?

[Laszlo Gombos]

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.

[Laszlo Gombos]

*** Bug 29725 has been marked as a duplicate of this bug. ***

[Laszlo Gombos]

(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.

[Laszlo Gombos]

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 ?

[Eric Seidel (no email)]

Comment on attachment 40091 [details]
2nd patch, independent of 1st.

The previous indent trick was easier to read.

[Eric Seidel (no email)]

CCing reviewers who actually know something about ARM (I don't).

[Gavin Barraclough]

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)

[Janne Koskinen]

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.

[Gabor Loki]

(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).

[Gabor Loki]

(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.

[Gabor Loki]

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.

[Gabor Loki]

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

[Gabor Loki]

Created attachment 40104 [details]
Check RVCT macros for ARM_THUMB2 platform

Ops, I attached a wrong patch.

This patch should fix the original problem.

[Laszlo Gombos]

(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.

[Yong Li]

> -#  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.

[Yong Li]

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

[WebKit Commit Bot]

Comment on attachment 40088 [details]
proposed patch

Clearing flags on attachment: 40088

Committed r48756: <http://trac.webkit.org/changeset/48756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gabor Loki]

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.

[Gabor Loki]

Unfortunately, the commit bot closed this bug entry, but the original problem is still present.

[Eric Seidel (no email)]

I think you're referring to bug 28230.  Sorry about that.

[Gavin Barraclough]

(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.

[Gabor Loki]

(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.

[George Wright]

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.

[George Staikos]

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+

[George Staikos]

Comment on attachment 40123 [details]
Check RVCT macros for ARM_THUMB2 platform (v2)

Clear review flag as there are obvious issues here.

[George Wright]

Created attachment 40397 [details]
RVCT macros fix - take 2

This revision moves the || operators to the left side as per the coding style guideline.

[George Wright]

Created attachment 40398 [details]
RVCT macros fix - take 3

Missed one || move.

[WebKit Commit Bot]

Comment on attachment 40398 [details]
RVCT macros fix - take 3

Clearing flags on attachment: 40398

Committed r48954: <http://trac.webkit.org/changeset/48954>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gabor Loki]

(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.