Closed Bug 618789 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox Build System :: General, defect)

ARM
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jbramley, Assigned: romaxa)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Add --enable-armv7. (obsolete) — Splinter Review
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.
Attachment #497239 - Flags: review?(blassey.bugs)
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
Attachment #497239 - Flags: review?(blassey.bugs) → review?(mitchell.field)
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.
Attachment #497239 - Flags: review?(mitchell.field)
Duplicate of this bug: 620239
Attachment #497239 - Attachment is obsolete: true
Attachment #502234 - Flags: review?
Attachment #502234 - Flags: review? → review?(mitchell.field)
Attachment #502234 - Flags: review?(mitchell.field) → review+
Attachment #502234 - Flags: approval2.0?
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
Assignee: Jacob.Bramley → romaxa
Attachment #502234 - Attachment is obsolete: true
Attachment #502314 - Flags: review?
Attachment #502234 - Flags: approval2.0?
Attachment #502314 - Flags: review?(mitchell.field)
Attachment #502314 - Flags: review?
Attachment #502314 - Flags: approval2.0?
Attachment #502314 - Flags: review?(mitchell.field) → review+
What's the risk/reward tradeoff of taking this now?
hmmm.. this kind of NPOTB
also this is regression.
Blocks: 577531
so can we get an approval for NPOTB?
Comment on attachment 502314 [details] [diff] [review]
Fixed compilation on non-arm arch

push to try first. dont break anything.
Attachment #502314 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/6de914cae124
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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.
> I've substituted it locally and nspr configure worked again on OS/2.

Could you create bug and attach ptach?
(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)?
> 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
Depends on: 626501
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.
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.
(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.
(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.
Attached patch NSPR patch (obsolete) — Splinter Review
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.
Attachment #507753 - Flags: superreview?(ted.mielczarek)
Attachment #507753 - Flags: review?(mh+mozilla)
(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.
(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.)
Let's continue this talk in bug 626035
Attachment #507753 - Attachment is obsolete: true
Attachment #507753 - Flags: superreview?(ted.mielczarek)
Attachment #507753 - Flags: review?(mh+mozilla)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.