Bug 29715

Patch?

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.

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

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

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 on attachment 40091 [details] 2nd patch, independent of 1st. The previous indent trick was easier to read.

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

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)

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.

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

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

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.

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

Created attachment 40104 [details] Check RVCT macros for ARM_THUMB2 platform Ops, I attached a wrong patch. This patch should fix the original problem.

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

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

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 on attachment 40088 [details] proposed patch Clearing flags on attachment: 40088 Committed r48756: <http://trac.webkit.org/changeset/48756>

All reviewed patches have been landed. Closing bug.

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.

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

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

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

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

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 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 on attachment 40123 [details] Check RVCT macros for ARM_THUMB2 platform (v2) Clear review flag as there are obvious issues here.

Created attachment 40397 [details] RVCT macros fix - take 2 This revision moves the || operators to the left side as per the coding style guideline.

Created attachment 40398 [details] RVCT macros fix - take 3 Missed one || move.

Comment on attachment 40398 [details] RVCT macros fix - take 3 Clearing flags on attachment: 40398 Committed r48954: <http://trac.webkit.org/changeset/48954>

All reviewed patches have been landed. Closing bug.

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