619481 - after binutils upgrade, 'xptcinvoke_arm.cpp' fails to compile for ARM thumb2 with hardfp ABI

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Siarhei Siamashka]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101030 Gentoo Firefox/3.6.12
Build Identifier: 

Newer versions of binutils (at least 2.21) started complaining about 'tst sp, #4' instruction when compiling firefox for thumb2. And according to ARM ARM, this instruction is indeed specified to have undefined behaviour in thumb2 mode even though it seems to work fine on real hardware.

armv7a-hardfloat-linux-gnueabi-g++ -o xptcinvoke_arm.o -c  -fvisibility=hidden -DMOZILLA_INTERNAL_API -DOSTYPE=\"Linux\" -DOSARCH=Linux -DEXPORT_XPTC_API -D_IMPL_NS_COM -I/mnt/igepv2-nfs-root/home/ssvb/projects/mozilla-central/xpcom/reflect/xptcall/src/md/unix/../.. -I/mnt/igepv2-nfs-root/home/ssvb/projects/mozilla-central/xpcom/reflect/xptcall/src/md/unix/../../../../xptinfo/src  -I/mnt/igepv2-nfs-root/home/ssvb/projects/mozilla-central/xpcom/reflect/xptcall/src/md/unix -I. -I../../../../../../dist/include -I../../../../../../dist/include/nsprpub  -I/mnt/igepv2-nfs-root/home/ssvb/projects/mozilla-central/objdir-ff-release/dist/include/nspr -I/mnt/igepv2-nfs-root/home/ssvb/projects/mozilla-central/objdir-ff-release/dist/include/nss       -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -pedantic -Wno-long-long -fno-strict-aliasing -fshort-wchar -pthread -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb -pipe  -DNDEBUG -DTRIMMED -Os -freorder-blocks -fomit-frame-pointer -finline-limit=50 -O2   -DMOZILLA_CLIENT -include ../../../../../../mozilla-config.h -MD -MF .deps/xptcinvoke_arm.pp /mnt/igepv2-nfs-root/home/ssvb/projects/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp
{standard input}: Assembler messages:
{standard input}:263: Error: r13 not allowed here -- `tst sp,#4'


Reproducible: Always

Comment 1

[Siarhei Siamashka]

Attachment: thumb2 hardfp compilation fix.

Proposed patch. Solves the problem at the cost of adding one extra instruction. This can be probably avoided by using separate code for arm and thumb2 mode via #ifdef.

Comment 2

[Jacob Bramley [:jbramley]]

Attachment: [UNTESTED] Re-jig hardfp NS_InvokeByIndex.

Oh, nasty. I'd forgotten that Thumb-2 restricted that instruction.

The additional MOV will work. However, in playing around with it I think I found a cleaner solution anyway. It doesn't require an extra instruction and does away with the conditionally-executed alignment code. There's room for making it more dual-issue-friendly, but I wanted this to be as readable as possible, at least initially.

(I don't mean to steal the bug, by the way. I'm just contributing what I was playing with.)

Actually, I'm a bit suspicious about the LDMDB...VLDM sequence near the end of the asm block. It doesn't look right to me. Does the VLDM not re-read into d0-d7 what was just read into r1-r3? I think I reviewed that code the first time around, but I can't see how it works now.

Comment 3

[Siarhei Siamashka]

(In reply to comment #2)
> The additional MOV will work. However, in playing around with it I think I
> found a cleaner solution anyway. It doesn't require an extra instruction and
> does away with the conditionally-executed alignment code. There's room for
> making it more dual-issue-friendly, but I wanted this to be as readable as
> possible, at least initially.

Agreed, this approach indeed looks better because it saves a few instructions.

> Actually, I'm a bit suspicious about the LDMDB...VLDM sequence near the end of
> the asm block. It doesn't look right to me. Does the VLDM not re-read into
> d0-d7 what was just read into r1-r3?

LDMDB is a "decrement before" variant of instruction, so it reads data below the address from IP register and does not clash with VLDM. So this part should be fine. And if it was wrong, it would have no chance to pass tests.

Comment 5

[Oleg Romashin (:romaxa)]

What else is needed to land this patch?

Comment 6

[Siarhei Siamashka]

Honestly I don't know regarding what is needed next.

My initial patch is a trivial and safe fix. The alternative method from Jacob is a rewrite of this part of code, which should also do the job and actually work a bit faster. But more intrusive changes need more careful testing and may have slightly higher risk of introducing regressions. I'm not sure myself what's the best plan.

Comment 7

[Oleg Romashin (:romaxa)]

Comment on attachment 497895 [details] [diff] [review]
thumb2 hardfp compilation fix.

Ok, I'll request approval for fast and safe fix then.

Comment 8

[Oleg Romashin (:romaxa)]

Can we get an approval here? without it we should add --disable-thumb2 in mozconfig all the time..

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

this looks safe enough code-wise. How have you tested it?

Comment 10

[Oleg Romashin (:romaxa)]

Yes, it does not break default builds, it is good on mobile try, and it allow to compile with thumb on hardfp.

In order to test it completely, I need to fix some other bugs (Qt related) which prevents us compile with thumb2. but I'm going to do that in followups..

http://hg.mozilla.org/mozilla-central/rev/3b6ff0e5ecff