618789 - Allow ARMv7 builds for ARM code (rather than Thumb-2 code).

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jacob Bramley [:jbramley]]

Attachment: Add --enable-armv7.

The attached patch adds '--enable-armv7' to the configure scripts. This
allows the build system to target ARMv7 without using Thumb-2.

The existing --enable-thumb2 now implies --enable-armv7, but if neither
are specified an ARMv5TE build is performed, as in the current
behaviour. If contradicting flags (--enable-thumb2 --disable-armv7) are
specified, configure will fail.

A complete list of possible combinations and their effects:

Flags                                       Result
--enable-thumb2     --enable-armv7          Thumb-2, ARMv7-A build.
--enable-thumb2                             Thumb-2, ARMv7-A build.
--enable-thumb2     --disable-armv7         Configure error.
--disable-thumb2    --enable-armv7          ARM, ARMv7-A build.
--disable-thumb2    --disable-armv7         ARM, ARMv5TE build.
--disable-thumb2                            ARM, ARMv5TE build.
                    --enable-armv7          ARM, ARMv7-A build.
                    --disable-armv7         ARM, ARMv5TE build.
                                            ARM, ARMv5TE build.

Note that building for Android currently changes the defaults as if
'--enable-thumb2' had been specified. My patch does not affect this.

Comment 1

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

Comment on attachment 497239 [details] [diff] [review]
Add --enable-armv7.

I'm not a build peer, you should get review from one of them.

As a drive by comment, you might want to do something like --with-cpu-arch=armv7 so it can be more generic and future proof.  That should default to armv7 for thumb2 and armv5te otherwise

Comment 2

[Mitchell Field [:Mitch]]

Comment on attachment 497239 [details] [diff] [review]
Add --enable-armv7.

This looks good but what blassey's suggesting makes sense. Implement that and I'll re-review.

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: fixed review comments. --with-cpu-arch

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Fixed compilation on non-arm arch

Previous version had default --with-cpu-arch=armv7 and was breaking non-arm arch.
Problem fixed in this version, also fixed some old wrong --enable-armv7 related error messages
Try server:
http://hg.mozilla.org/try/rev/8df5e84660bd

Comment 6

[Benjamin Smedberg]

What's the risk/reward tradeoff of taking this now?

Comment 7

[Oleg Romashin (:romaxa)]

hmmm.. this kind of NPOTB

Comment 8

[Oleg Romashin (:romaxa)]

also this is regression.

Comment 9

[Oleg Romashin (:romaxa)]

so can we get an approval for NPOTB?

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 502314 [details] [diff] [review]
Fixed compilation on non-arm arch

push to try first. dont break anything.

Comment 11

[Oleg Romashin (:romaxa)]

try build:
http://hg.mozilla.org/try/rev/796528801485
looks good

Comment 12

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/6de914cae124

Comment 13

[Walter Meinl]

(In reply to comment #10)
> Comment on attachment 502314 [details] [diff] [review]
> Fixed compilation on non-arm arch
> 
> push to try first. dont break anything.

In fact, it broke OS/2 in nsprpub configure. Note, that the arm changes were inserted right after the OS/2 High-mem check. Now, configure bails out with a wording error: expected ")")
The reason appears to be that nspr configure / m4 hasn't defined the macro
MOZ_ARG_WITH_STRING. It should be AC_ARG_WITH
I've substituted it locally and nspr configure worked again on OS/2.

Comment 14

[Oleg Romashin (:romaxa)]

> I've substituted it locally and nspr configure worked again on OS/2.

Could you create bug and attach ptach?

Comment 15

[Walter Meinl]

(In reply to comment #14)
> > I've substituted it locally and nspr configure worked again on OS/2.
> 
> Could you create bug and attach ptach?

Should the patch in CVS (nspr repo) or HG format (mozilla-central)?

Comment 16

[Oleg Romashin (:romaxa)]

> Should the patch in CVS (nspr repo) or HG format (mozilla-central)?

I don't know... report bug and ask nspr people.... this bug has been pushed into mercurial... so I guess new patch should apply to mercurial repo

Comment 17

[(not currently active) Ted Mielczarek]

romaxa: you're not supposed to land NSPR changes directly in mozilla-central. They're supposed to land in NSPR CVS, and then we take a snapshot of that into mozilla-central.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I reverted us to NSPR 4.8.7 RTM in http://hg.mozilla.org/mozilla-central/rev/ac1ddab6de59, effectively backing out the NSPR portions of this patch.

Comment 19

[Wan-Teh Chang]

(In reply to comment #15)
>
> Should the patch in CVS (nspr repo) or HG format (mozilla-central)?

Hi Walter,

Please generate the NSPR patch in CVS format.  It's better to open
a new NSPR bug and attach the patch there, but you can also just
attach the patch to this bug.  Thanks.

Comment 20

[Walter Meinl]

(In reply to comment #19)
> (In reply to comment #15)
> >
> > Should the patch in CVS (nspr repo) or HG format (mozilla-central)?
> 
> Hi Walter,
> 
> Please generate the NSPR patch in CVS format.  It's better to open
> a new NSPR bug and attach the patch there, but you can also just
> attach the patch to this bug.  Thanks.

Hi Wan-Teh,
thanks for your advise, I wouldn't have asked, if the patch in question were also made for the nspr-repo. However, since the patch was pushed directly to mozilla-central, I was unsure. Now, the nspr-part was reverted and it depends on romaxa if he wants to get those bits also to nspr.

Comment 21

[Wan-Teh Chang]

Attachment: NSPR patch

This patch contains the NSPR changes in attachment 502314 [details] [diff] [review],
updated to the NSPR CVS trunk and including Walter Meinl's
OS/2 build fix.

--with-cpu-arch is a poor name because this option is ARM
specific, and configure.in already has a variable named
CPU_ARCH.  This option should be renamed.  How about
--with-arm-arch?

glandium: in bug 626035 comment 7 you said MOZ_ARM_ARCH
should default to nothing.  I don't understand your comment,
which is why I asked you to review this.

Comment 22

[Mike Hommey [:glandium]]

(In reply to comment #21)
> glandium: in bug 626035 comment 7 you said MOZ_ARM_ARCH
> should default to nothing.  I don't understand your comment,
> which is why I asked you to review this.

A given toolchain has a default target. On some, the default is armv7, on others it's armv5t, and on yet others, it's armv4t.

Actually, the more I think about it, the more I think the whole thing is just wrong.
MOZ_ARM_ARCH should default to nothing, leaving whatever the toolchain uses by default, and --with-arm-arch could be used to give the -march value to give to the toolchain, whatever it is.

Comment 23

[Jacob Bramley [:jbramley]]

(In reply to comment #22)
> MOZ_ARM_ARCH should default to nothing, leaving whatever the toolchain uses by
> default, and --with-arm-arch could be used to give the -march value to give to
> the toolchain, whatever it is.

I agree with your statement. The default build should use the default tool-chain options, provided that we can override them. (-march by itself is not sufficent, though. We need either -mcpu, or -march and -mtune, and in both cases we probably need -mfloat-abi and -mfpu.)

I always used to achieve this by defining {C,CXX,CPP}FLAGS in the environment when invoking configure. (My compiler defaulted to ARMv5T without VFP, but I wanted ARMv7 builds with VFP.)

Comment 24

[Mike Hommey [:glandium]]

Let's continue this talk in bug 626035