Last Comment Bug 626035 - -march=armv5+ -mthumb* forced on all arm platforms
: -march=armv5+ -mthumb* forced on all arm platforms
Status: RESOLVED FIXED
:
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: 4.8.7
: ARM Linux
: P1 normal (vote)
: 4.8.9
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
: 656449 (view as bug list)
Depends on: 636701 674837
Blocks: 547946 632915 646846
  Show dependency treegraph
 
Reported: 2011-01-15 02:59 PST by Mike Hommey [:glandium]
Modified: 2011-07-28 02:19 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Only add -march=armv5te -mthumb-interwork on android (checked in to NSPR CVS trunk) (445 bytes, patch)
2011-01-15 02:59 PST, Mike Hommey [:glandium]
wtc: review+
blassey.bugs: review+
Details | Diff | Splinter Review
Only add -march=armv5te -mthumb-interwork on android (for m-c) (1.51 KB, patch)
2011-01-15 03:01 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
New proposal (5.71 KB, patch)
2011-01-28 01:12 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Alternative proposal (6.91 KB, patch)
2011-01-31 01:41 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Updated version of alternative patch, extended to js (12.86 KB, patch)
2011-02-09 12:52 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Updated, tested and compiled. (12.87 KB, patch)
2011-02-10 01:43 PST, Oleg Romashin (:romaxa)
ted: review+
Details | Diff | Splinter Review
Use toolchain defaults on ARM and let users set their own flags through environment (14.19 KB, patch)
2011-03-03 02:28 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
Use toolchain defaults on ARM and let users set their own flags through environment, nspr part (2.34 KB, patch)
2011-03-03 02:35 PST, Mike Hommey [:glandium]
wtc: review+
Details | Diff | Splinter Review
Use toolchain defaults on ARM and let users set their own flags through environment, nspr part (checked in, then backed out) (2.49 KB, patch)
2011-03-08 08:14 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure (22.56 KB, patch)
2011-04-18 08:00 PDT, Mike Hommey [:glandium]
romaxa: feedback+
blassey.bugs: feedback+
Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure. nspr part (6.01 KB, patch)
2011-04-18 08:01 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure (22.58 KB, patch)
2011-04-19 00:14 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure. nspr part (6.02 KB, patch)
2011-04-19 00:15 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure (23.15 KB, patch)
2011-04-26 23:59 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure (23.62 KB, patch)
2011-05-02 08:11 PDT, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure - NSPR part (6.42 KB, patch)
2011-05-02 08:13 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
some intermediate patch (45.06 KB, patch)
2011-05-04 07:11 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure - NSPR part (72.76 KB, patch)
2011-05-04 08:26 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure (23.75 KB, patch)
2011-06-01 03:19 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Modify the way arm compiler flags are set in configure - NSPR part (72.74 KB, patch)
2011-06-01 03:21 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2011-01-15 02:59:27 PST
Created attachment 504094 [details] [diff] [review]
Only add  -march=armv5te -mthumb-interwork on android (checked in to NSPR CVS trunk)

Before bug 577531, only android targets would force using -march=armv5te -mthumb-interwork. After bug 577531, you either get -march=armv5te -mthumb-interwork or -march=armv7-a -mthumb depending whether you enabled thumb2 on all arm platforms using GCC. Other platforms should use the gcc target, whatever it is.
Comment 1 Mike Hommey [:glandium] 2011-01-15 03:01:57 PST
Created attachment 504095 [details] [diff] [review]
Only add  -march=armv5te -mthumb-interwork on android (for m-c)

Same patch, for m-c
Comment 2 Mike Hommey [:glandium] 2011-01-15 04:32:14 PST
This is great, bug 618789 made it even worse.
Comment 3 Mike Hommey [:glandium] 2011-01-15 04:34:53 PST
So, the new current state is that MOZ_ARM_ARCH has no meaningful purpose, since any value other than armv7 is discarded to use armv5te instead. What's wrong with defaulting to whatever the gcc target is ?
Comment 4 Wan-Teh Chang 2011-01-15 10:01:50 PST
Comment on attachment 504094 [details] [diff] [review]
Only add  -march=armv5te -mthumb-interwork on android (checked in to NSPR CVS trunk)

r=wtc.

blassey: could you also review this change?  It tweaks
your patch for bug 577531.
Comment 5 Wan-Teh Chang 2011-01-21 16:03:44 PST
blassey: ping!
Comment 6 Wan-Teh Chang 2011-01-21 19:06:27 PST
Mike, is this bug a duplicate of bug 616413?
Comment 7 Mike Hommey [:glandium] 2011-01-22 00:05:09 PST
(In reply to comment #6)
> Mike, is this bug a duplicate of bug 616413?

I don't think it is. Please note, though that the patch here is not enough now, because of the changes from bug 618789. I think MOZ_ARM_ARCH should default to nothing.
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2011-01-22 07:13:37 PST
Comment on attachment 504094 [details] [diff] [review]
Only add  -march=armv5te -mthumb-interwork on android (checked in to NSPR CVS trunk)

This is fine. More of a meta comment it seems like this has gotten overly complicated.
Comment 9 Wan-Teh Chang 2011-01-27 18:00:41 PST
Comment on attachment 504094 [details] [diff] [review]
Only add  -march=armv5te -mthumb-interwork on android (checked in to NSPR CVS trunk)

Patch checked in on the NSPR trunk (NSPR 4.8.8).

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.293; previous revision: 1.292
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.296; previous revision: 1.295
done
Comment 10 Mike Hommey [:glandium] 2011-01-28 01:12:50 PST
Created attachment 507806 [details] [diff] [review]
New proposal

This is a new proposal, as per bug 618789 comment 22. It is untested and only contains main m-c configure.in part. I'll complete the patch and test it after getting feedback.

The intent is the following:
- If you give a --with-arm-arch value, it will use it as -march argument to gcc.
- If you don't give a --with-arm-arch value, it will keep the compiler default.

- If you give --enable-thumb2, it will add -mthumb to your flags and make sure you provided an armv7 target with --with-arm-arch, or you you didn't give one, will default to --with-arm-arch=armv7-a.
- If you explicitely --disable-thumb2, it will add -marm to your flags.
- If you don't specify either --enable-thumb2 or --disable-thumb2, it will add -mthumb-interwork to your flags.

I already know that the argument passing to sub-configure won't work for --disable-thumb2 with this patch.
Comment 11 Mike Hommey [:glandium] 2011-01-28 01:19:13 PST
(In reply to bug 618789 comment #23)
> 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.)

So maybe all we need is some kind of generic way to make that easier, like, setting one variable, which would propagate to these variables.

e.g. COMPILE_FLAGS=-march=armv7-a would set
CFLAGS+=-march=armv7-a -Wa,-march=armv7-a
CXXFLAGS+=-march=armv7-a -Wa,-march=armv7-a
ASFLAGS+=-march=armv7-a

We'd probably need a whitelist of flags for automatic setting of -Wa, arguments.
Comment 12 Jacob Bramley [:jbramley] 2011-01-28 01:41:34 PST
Out of interest, why is it necessary to set the assembly flags via -Wa? I've not done that before. Doesn't GCC pass these flags through anyway?

(In reply to comment #11)
> So maybe all we need is some kind of generic way to make that easier, like,
> setting one variable, which would propagate to these variables.

Agreed.
Comment 13 Mike Hommey [:glandium] 2011-01-28 02:00:20 PST
(In reply to comment #12)
> Out of interest, why is it necessary to set the assembly flags via -Wa? I've
> not done that before. Doesn't GCC pass these flags through anyway?

It should, and actually, considering bug 627299, it worked just fine without. I don't know why romaxa added them. I just kept them around because I assumed there was something adding them was supposed to achieve.
Comment 14 Mike Hommey [:glandium] 2011-01-28 02:03:09 PST
(In reply to comment #13)
> why romaxa added them

Actually, that was blassey, in bug 563751
Comment 15 Mike Hommey [:glandium] 2011-01-29 00:53:59 PST
Only downside is that there are a few places that do use the MOZ_THUMB2 variable:
js/src/methodjit/MethodJIT.cpp
media/libtremor/lib/Makefile.in
toolkit/mozapps/installer/packager.mk

... though we could probably replace that with $(filter -mthumb,$(CFLAGS)) or something like that.
Comment 16 Mike Hommey [:glandium] 2011-01-29 01:09:33 PST
(In reply to comment #15)
> ... though we could probably replace that with $(filter -mthumb,$(CFLAGS)) or
> something like that.


Or even better, detect #ifdef __thumb__ with an AC_TRY_COMPILE
Comment 17 Brad Lassey [:blassey] (use needinfo?) 2011-01-29 08:39:08 PST
(In reply to comment #15)
> ... though we could probably replace that with $(filter -mthumb,$(CFLAGS)) or
> something like that.

gotta watch out for -mthumb-interwork when testing cflags
Comment 18 Mike Hommey [:glandium] 2011-01-31 01:41:27 PST
Created attachment 508348 [details] [diff] [review]
Alternative proposal

Again, untested, and only including main configure.in changes. This time, we just remove the --enable/--with flags. People would be expected to set their *FLAGS variables appropriately depending on their compiler and wanted target. The configure script would however detect if these flags mean building for Thumb-2.

This also means build bots configuration would first need to be adapted.
Comment 19 Oleg Romashin (:romaxa) 2011-02-09 12:52:04 PST
Created attachment 511151 [details] [diff] [review]
Updated version of alternative patch, extended to js
Comment 20 Oleg Romashin (:romaxa) 2011-02-09 13:06:18 PST
Comment on attachment 511151 [details] [diff] [review]
Updated version of alternative patch, extended to js

oh, this actually still does not do the right things for hardfp...
Comment 21 Mike Hommey [:glandium] 2011-02-09 13:09:01 PST
(In reply to comment #20)
> Comment on attachment 511151 [details] [diff] [review]
> Updated version of alternative patch, extended to js
> 
> oh, this actually still does not do the right things for hardfp...

Because you need to set the flags yourself in your .mozconfig.
Comment 22 Oleg Romashin (:romaxa) 2011-02-10 01:43:02 PST
Created attachment 511322 [details] [diff] [review]
Updated, tested and compiled.

Ok, I got mozilla compiled on harfp toolchain with this patch + next lines in mozconfig:
**************************
ac_cv_gcc_arm_thumb2="no"
CFLAGS="-march=armv7-a -marm"
CXXFLAGS="-march=armv7-a -marm"
ASFLAGS="-march=armv7-a -marm"
**************************

it works good for me
Comment 23 Mike Hommey [:glandium] 2011-02-10 01:49:16 PST
(In reply to comment #22)
> Created attachment 511322 [details] [diff] [review]
> Updated, tested and compiled.
> 
> Ok, I got mozilla compiled on harfp toolchain with this patch + next lines in
> mozconfig:
> **************************
> ac_cv_gcc_arm_thumb2="no"
> CFLAGS="-march=armv7-a -marm"
> CXXFLAGS="-march=armv7-a -marm"
> ASFLAGS="-march=armv7-a -marm"
> **************************
> 
> it works good for me

The check for thumb2 should be using CFLAGS, and as such should work properly if you give -marm, without setting ac_cv_gcc_arm_thumb2="no". If that doesn't work, then there is something wrong.
Comment 24 Oleg Romashin (:romaxa) 2011-02-10 03:32:27 PST
tested without setting - ac_cv_gcc_arm_thumb2="no", and it also works fine.
Comment 25 Mike Hommey [:glandium] 2011-02-10 08:05:01 PST
Comment on attachment 511322 [details] [diff] [review]
Updated, tested and compiled.

I'm obviously not going to review a patch that I wrote at 95%...
Comment 26 Mike Hommey [:glandium] 2011-02-10 08:05:26 PST
Note we should do the same in nsprpub, too.
Comment 27 Mike Hommey [:glandium] 2011-02-10 08:06:29 PST
And before getting this to land, we need to have build people update mozconfigs for arm builds.
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-02-11 13:55:22 PST
Comment on attachment 511322 [details] [diff] [review]
Updated, tested and compiled.

Punting
Comment 29 Ted Mielczarek [:ted.mielczarek] 2011-02-25 05:01:38 PST
Comment on attachment 511322 [details] [diff] [review]
Updated, tested and compiled.

This looks nicer, but this can't land until we fix the mozconfigs. Please file a bug blocking this one for that change.
Comment 30 Mike Hommey [:glandium] 2011-03-03 02:28:30 PST
Created attachment 516547 [details] [diff] [review]
Use toolchain defaults on ARM and let users set their own flags through environment

Same patch, removing the setting of MOZ_ARM_VFP_FLAGS which is now unused. Carrying over r+
Comment 31 Mike Hommey [:glandium] 2011-03-03 02:35:49 PST
Created attachment 516548 [details] [diff] [review]
Use toolchain defaults on ARM and let users set their own flags through environment, nspr part

Apparently, removing the whole thing in nspr should be enough.
Comment 32 Wan-Teh Chang 2011-03-03 10:11:47 PST
Comment on attachment 516548 [details] [diff] [review]
Use toolchain defaults on ARM and let users set their own flags through environment, nspr part

r=wtc.

Just to confirm: the NSPR patch is very different from the changes to
configure.in and js/src/configure.in in attachment 516547 [details] [diff] [review], because
NSPR doesn't use either the MOZ_THUMB2 makefile variable or the
MOZ_THUMB2 C-preprocessor macro, right?
Comment 33 Mike Hommey [:glandium] 2011-03-03 10:19:18 PST
(In reply to comment #32)
> Comment on attachment 516548 [details] [diff] [review]
> Use toolchain defaults on ARM and let users set their own flags through
> environment, nspr part
> 
> r=wtc.
> 
> Just to confirm: the NSPR patch is very different from the changes to
> configure.in and js/src/configure.in in attachment 516547 [details] [diff] [review], because
> NSPR doesn't use either the MOZ_THUMB2 makefile variable or the
> MOZ_THUMB2 C-preprocessor macro, right?

exact.
Comment 34 Wan-Teh Chang 2011-03-08 08:14:02 PST
Created attachment 517747 [details] [diff] [review]
Use toolchain defaults on ARM and let users set their own flags through environment, nspr part (checked in, then backed out)

I updated the patch to the NSPR trunk (NSPR 4.8.8) and
checked it in.

Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.297; previous revision: 1.296
done
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.294; previous revision: 1.293
done
Comment 35 Wan-Teh Chang 2011-03-08 10:44:04 PST
Mike, do you still want the patches in this bug checked in?
I am confused by your marking bug 636701 as INVALID.

I am preparing the NSPR 4.8.8 release today, so I need to
know if it can include the NSPR patch in this bug.  Thanks.
Comment 36 Mike Hommey [:glandium] 2011-03-08 10:55:07 PST
blassey doesn't want the mozconfigs to contain CFLAGS because of the implications that would have on developer environments.

However, I do believe that nspr should be exempt of these requirements, and that the mozilla codebase can pass the flags it requires at configure time with the *FLAGS variables instead of relying on --enable arguments. If there's a bug for a nspr 4.8.8 merge on m-c or another branch, though, it should depend on this bug.

Brad, do you agree here?
Comment 37 Brad Lassey [:blassey] (use needinfo?) 2011-03-08 11:13:15 PST
That seems reasonable, however, given that we're 13 hours from code freeze for Fennec's RC, I'd prefer we not take this for 4.8.8 unless its been tested thoroughly.
Comment 38 Mike Hommey [:glandium] 2011-03-08 11:35:36 PST
If Fennec RC going to merge nspr 4.8.8 ?
Comment 39 Brad Lassey [:blassey] (use needinfo?) 2011-03-08 11:36:37 PST
that's the plan (see bug 639191)
Comment 40 Mike Hommey [:glandium] 2011-03-08 11:42:54 PST
(In reply to comment #37)
> That seems reasonable, however, given that we're 13 hours from code freeze for
> Fennec's RC, I'd prefer we not take this for 4.8.8 unless its been tested
> thoroughly.

With the current configure.in in m-c, sub-configure commands take *FLAGS as they were passed to the main configure, so the landed patch is going to be a problem.
Comment 41 Mike Hommey [:glandium] 2011-03-08 11:45:33 PST
(In reply to comment #40)
> (In reply to comment #37)
> > That seems reasonable, however, given that we're 13 hours from code freeze for
> > Fennec's RC, I'd prefer we not take this for 4.8.8 unless its been tested
> > thoroughly.
> 
> With the current configure.in in m-c, sub-configure commands take *FLAGS as
> they were passed to the main configure, so the landed patch is going to be a
> problem.

OTOH, the code in nsprpub's configure.in before the landing is not really suitable with a generalist release: it enforces armv5te and thumb interwork by default.
Comment 42 Wan-Teh Chang 2011-03-08 12:27:30 PST
Mike, Brad: I'll let you figure out what to do for NSPR.

Just a reminder that NSPR can be built as a standalone library,
independent of Mozilla.  We cannot assume NSPR's configure script
is always a "sub-configure" driven by the "main" configure.
Comment 43 Mike Hommey [:glandium] 2011-03-08 12:49:10 PST
(In reply to comment #42)
> Mike, Brad: I'll let you figure out what to do for NSPR.
> 
> Just a reminder that NSPR can be built as a standalone library,
> independent of Mozilla.  We cannot assume NSPR's configure script
> is always a "sub-configure" driven by the "main" configure.

From the perspective of a standalone library, what you landed is just the right thing. It just doesn't fit with the current main configure.
Comment 44 Wan-Teh Chang 2011-03-09 08:08:49 PST
Comment on attachment 517747 [details] [diff] [review]
Use toolchain defaults on ARM and let users set their own flags through environment, nspr part (checked in, then backed out)

Forcing developers to set non-obvious compiler flags as
shown in attachment 516550 [details] [diff] [review] does seem inconvenient.
NSPR's build system has always embedded the recommended
platform-specific compiler flags and hides the complexity
under configure options such as --enable-optimize and
--enable-64bit.

I backed out this patch on the NSPR trunk (NSPR 4.8.8).

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.295; previous revision: 1.294
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.298; previous revision: 1.297
done
Comment 45 Oleg Romashin (:romaxa) 2011-03-12 14:18:09 PST
We need also remove this hardcoded part:
http://mxr.mozilla.org/mozilla-central/source/configure.in#3681

othervise it break NEON detection on hardfp toolchain
configure:11426: gcc -o conftest -gdwarf-2 -march=armv7-a -marm -fno-strict-aliasing -mfpu=neon -mfloat-abi=softfp   conftest.c -ldl  1>&5
ld: error: /tmp/cceCUjqb.o uses VFP register arguments, conftest does not
ld: failed to merge target specific data of file /tmp/cceCUjqb.o
collect2: ld returned 1 exit status
configure: failed program was:
Comment 46 Oleg Romashin (:romaxa) 2011-03-30 22:35:51 PDT
*** Bug 632915 has been marked as a duplicate of this bug. ***
Comment 47 Oleg Romashin (:romaxa) 2011-03-31 23:44:41 PDT
Any chance to get this patch landed?
Comment 48 Mike Hommey [:glandium] 2011-03-31 23:47:51 PDT
blassey doesn't want it.
Comment 49 Mike Hommey [:glandium] 2011-03-31 23:56:46 PDT
(In reply to comment #45)
> We need also remove this hardcoded part:
> http://mxr.mozilla.org/mozilla-central/source/configure.in#3681
> 
> othervise it break NEON detection on hardfp toolchain
> configure:11426: gcc -o conftest -gdwarf-2 -march=armv7-a -marm
> -fno-strict-aliasing -mfpu=neon -mfloat-abi=softfp   conftest.c -ldl  1>&5
> ld: error: /tmp/cceCUjqb.o uses VFP register arguments, conftest does not
> ld: failed to merge target specific data of file /tmp/cceCUjqb.o
> collect2: ld returned 1 exit status
> configure: failed program was:

That is bug 547946
Comment 50 Mike Hommey [:glandium] 2011-04-01 00:06:59 PDT
So, let's try to summarize the problem.
- We have some flags that need to be used to build e.g. some NEON stuff for runtime detection (ycbcr (though it doesn't do runtime detection at the moment, it could), vp8)
- We have some other flags used to use a specific general build target.
- We have basically 3 main consumers on ARM: fennec/android, fennec/maemo, linux distros.
- Some people may want to try to build with a different target than the defaults (there seem to be people wanting to e.g. build fennec/android for armv6)

So, all in all, this may be what we need:
- Have configure flags for the main build flags: --enable-thumb/--disable-thumb, --with-arm-arch=something. We may want other configure flags for float abi.
- Default to nothing on plain linux (i.e. let the toolchain decide)
- Have configure set defaults for Android and Maemo (but still allow to override them ; we may want to allow things such as --with-arm-arch=default and --enable-thumb=maybe)

Brad, would you be satisfied with the above?
Comment 51 Jeremias Bosch (:jbos) 2011-04-13 01:48:54 PDT
mhm can we somehow get this (https://bug626035.bugzilla.mozilla.org/attachment.cgi?id=516547) patch landed? without it i get configure issues 

checking size of int *... 0
configure: error: Unexpected pointer size
Comment 52 Mike Hommey [:glandium] 2011-04-18 08:00:26 PDT
Created attachment 526726 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

This follows what I wrote in comment 50, except for the option names and values. The value "toolchain" passed to these arguments is used to override the defaults set for android and maemo. Plain ARM linux defaults to use no specific flags.

It also fixes bug 632915, bug 646846 and bug 547946.
Comment 53 Mike Hommey [:glandium] 2011-04-18 08:01:32 PDT
Created attachment 526727 [details] [diff] [review]
Modify the way arm compiler flags are set in configure. nspr part
Comment 54 Brad Lassey [:blassey] (use needinfo?) 2011-04-18 09:18:04 PDT
Comment on attachment 526726 [details] [diff] [review]
Modify the way arm compiler flags are set in configure


>+MOZ_ARG_WITH_STRING(thumb,
>+[  --with-thumb-interwork[[=yes|no|toolchain]]
>+                           Use Thumb/ARM instuctions interwork (-mthumb-interwork)],
I believe this should be MOZ_ARG_WITH_STRING(thumb-interwork,


>+case "$MOZ_THUMB" in
>+yes)
>+    MOZ_THUMB2=1
>+    thumb_flag="-mthumb"
>+    ;;
>+no)
>+    MOZ_THUMB2=
>+    thumb_flag="-marm"
>+    ;;
>+*)
>+    _SAVE_CFLAGS="$CFLAGS"
>+    CFLAGS="$arch_flag"
>+    AC_TRY_COMPILE([],[return sizeof(__thumb2__);],
>+        MOZ_THUMB2=1,
>+        MOZ_THUMB2=)
>+    CFLAGS="$_SAVE_CFLAGS"
>+    thumb_flag=""
>+    ;;
>+esac

why have MOZ_THUMB and MOZ_THUMB2? 

this looks good to me otherwise, but ted should probably review it
Comment 55 Brad Lassey [:blassey] (use needinfo?) 2011-04-18 09:33:44 PDT
Comment on attachment 526726 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

>+    if test "$MOZ_PLATFORM_MAEMO" = 6; then
>+        MOZ_THUMB=yes
>+    fi

are you sure about this? I thought maemo couldn't handle thumb2
Comment 56 Mike Hommey [:glandium] 2011-04-18 09:56:09 PDT
(In reply to comment #54)
> Comment on attachment 526726 [details] [diff] [review]
> Modify the way arm compiler flags are set in configure
> 
> 
> >+MOZ_ARG_WITH_STRING(thumb,
> >+[  --with-thumb-interwork[[=yes|no|toolchain]]
> >+                           Use Thumb/ARM instuctions interwork (-mthumb-interwork)],
> I believe this should be MOZ_ARG_WITH_STRING(thumb-interwork,

Obviously, yes.

> >+case "$MOZ_THUMB" in
> >+yes)
> >+    MOZ_THUMB2=1
> >+    thumb_flag="-mthumb"
> >+    ;;
> >+no)
> >+    MOZ_THUMB2=
> >+    thumb_flag="-marm"
> >+    ;;
> >+*)
> >+    _SAVE_CFLAGS="$CFLAGS"
> >+    CFLAGS="$arch_flag"
> >+    AC_TRY_COMPILE([],[return sizeof(__thumb2__);],
> >+        MOZ_THUMB2=1,
> >+        MOZ_THUMB2=)
> >+    CFLAGS="$_SAVE_CFLAGS"
> >+    thumb_flag=""
> >+    ;;
> >+esac
> 
> why have MOZ_THUMB and MOZ_THUMB2?

No reason other than the fact that I initially thought I'd need to keep the MOZ_THUMB value around to pass to sub-configure, but i actually didn't.
 
> this looks good to me otherwise, but ted should probably review it

I was planning to get ted's review, but wanted to first get your impressions.

(In reply to comment #55)
> Comment on attachment 526726 [details] [diff] [review]
> Modify the way arm compiler flags are set in configure
> 
> >+    if test "$MOZ_PLATFORM_MAEMO" = 6; then
> >+        MOZ_THUMB=yes
> >+    fi
> 
> are you sure about this? I thought maemo couldn't handle thumb2

See http://mxr.mozilla.org/mozilla-central/source/configure.in#7076
Comment 57 Oleg Romashin (:romaxa) 2011-04-18 11:41:09 PDT
Comment on attachment 526726 [details] [diff] [review]
Modify the way arm compiler flags are set in configure


>+dnl Defaults
>+case "${target}" in
>+arm-android-eabi)
>+    MOZ_THUMB=yes
>+    MOZ_ARCH=armv7-a
>+    MOZ_FPU=vfp
>+    MOZ_FLOAT_ABI=softfp
>+    ;;
>+arm*-*)
>+    if test -n "$MOZ_PLATFORM_MAEMO"; then
>+        MOZ_THUMB=no
>+        MOZ_ARCH=armv7-a
>+        MOZ_FLOAT_ABI=softfp
>+    fi
>+    if test "$MOZ_PLATFORM_MAEMO" = 6; then
>+        MOZ_THUMB=yes
>+    fi

this will force enable softfp for all maemo platforms... and I don't think this is right.
it still make sense to check for __ARM_PCS_VFP define and if that defined then disable hardcoded softfp by default.

http://gcc.gnu.org/viewcvs?view=revision&revision=162637
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45886
Comment 58 Oleg Romashin (:romaxa) 2011-04-18 11:49:06 PDT
Comment on attachment 526726 [details] [diff] [review]
Modify the way arm compiler flags are set in configure


>+    if test "$MOZ_PLATFORM_MAEMO" = 6; then
>+        MOZ_THUMB=yes
>+    fi
>+    ;;
Also please remove THUMB enabler for Maemo6, it does not work, due to  different Qt include/defines problems. this will possibly work in some future, but not now.
Comment 59 Siarhei Siamashka 2011-04-18 12:01:00 PDT
(In reply to comment #57)
> it still make sense to check for __ARM_PCS_VFP define and if that defined then
> disable hardcoded softfp by default.
> 
> http://gcc.gnu.org/viewcvs?view=revision&revision=162637
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45886

Maybe it's a better idea to do linking based test (via AC_TRY_LINK) trying to figure out the correct floating point ABI for the target system? When using the wrong ABI, test will fail exactly on the linking stage.
Comment 60 Oleg Romashin (:romaxa) 2011-04-18 17:46:39 PDT
Comment on attachment 526726 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

I like this approach, and it works fine with FLOAT_ABI config-time detection
Comment 61 Mike Hommey [:glandium] 2011-04-18 23:31:26 PDT
(In reply to comment #57)
> this will force enable softfp for all maemo platforms... and I don't think this
> is right.
> it still make sense to check for __ARM_PCS_VFP define and if that defined then
> disable hardcoded softfp by default.

This is a default you can override with --with-float-abi=toolchain to use what the toolchain uses or --with-float-abi=hardfp.

(In reply to comment #58)
> Comment on attachment 526726 [details] [diff] [review]
> Modify the way arm compiler flags are set in configure
> 
> 
> >+    if test "$MOZ_PLATFORM_MAEMO" = 6; then
> >+        MOZ_THUMB=yes
> >+    fi
> >+    ;;
> Also please remove THUMB enabler for Maemo6, it does not work, due to 
> different Qt include/defines problems. this will possibly work in some future,
> but not now.

I've been careful to make the defaults in this patch the same as they are now. The good thing about this patch is that now all defaults are overridable with configure options, which wasn't the case before.

I'm inclined to think this should land as-is (modulo the thumb-interwork typo), and changes to the defaults should be treated in a followup bug.
Comment 62 Mike Hommey [:glandium] 2011-04-19 00:14:30 PDT
Created attachment 526941 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

With the thumb-interwork typo fixed.
Comment 63 Mike Hommey [:glandium] 2011-04-19 00:15:24 PDT
Created attachment 526942 [details] [diff] [review]
Modify the way arm compiler flags are set in configure. nspr part
Comment 64 Mike Hommey [:glandium] 2011-04-26 23:59:46 PDT
Created attachment 528540 [details] [diff] [review]
Modify the way arm compiler flags are set in configure
Comment 65 Ted Mielczarek [:ted.mielczarek] 2011-04-29 07:31:29 PDT
Comment on attachment 528540 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

Review of attachment 528540 [details] [diff] [review]:

r=me with a few things addressed as noted. It looks better than what's currently there, at least.

::: configure.in
@@ +6945,5 @@
+dnl Kept for compatibility with some buildbot mozconfig
+MOZ_ARG_DISABLE_BOOL(thumb2, [], MOZ_THUMB=no, MOZ_THUMB=yes)
+
+MOZ_ARG_WITH_STRING(thumb,
+[  --with-thumb[[=yes|no|toolchain]]]

Can you document what "toolchain" means here? If this is "toolchain default", I would think "default" might be a better name, I'm not sure. Then again, what's the difference between leaving this option out, and specifying "--with-thumb=toolchain"? Is it that we may override the toolchain defaults in configure, so specifying =toolchain means "don't do that"? That's kind of confusing, and I'm not sure that that distinction is going to be easy to convey to users.

@@ +6954,5 @@
+    MOZ_THUMB=$withval)
+
+MOZ_ARG_WITH_STRING(thumb-interwork,
+[  --with-thumb-interwork[[=yes|no|toolchain]]
+                           Use Thumb/ARM instuctions interwork (-mthumb-interwork)],

This feels like it'd be better suited as a MOZ_ARG_ENABLE_BOOL, since the only options are enable/disable.

@@ +6986,5 @@
+    MOZ_FLOAT_ABI=$withval)
+
+MOZ_ARG_WITH_STRING(soft-float,
+[  --with-soft-float[[=yes|no|toolchain]]
+                           Use soft float library (-msoft-float)],

This seems like it should also be MOZ_ARG_ENABLE_BOOL.
Comment 66 Mike Hommey [:glandium] 2011-04-29 08:13:32 PDT
(In reply to comment #65)
> Comment on attachment 528540 [details] [diff] [review]
> Modify the way arm compiler flags are set in configure
> 
> Review of attachment 528540 [details] [diff] [review]:
> 
> r=me with a few things addressed as noted. It looks better than what's
> currently there, at least.
> 
> ::: configure.in
> @@ +6945,5 @@
> +dnl Kept for compatibility with some buildbot mozconfig
> +MOZ_ARG_DISABLE_BOOL(thumb2, [], MOZ_THUMB=no, MOZ_THUMB=yes)
> +
> +MOZ_ARG_WITH_STRING(thumb,
> +[  --with-thumb[[=yes|no|toolchain]]]
> 
> Can you document what "toolchain" means here? If this is "toolchain default", I
> would think "default" might be a better name, I'm not sure.

default would convey a different meaning IMHO, in that I would expect "default" to mean "whatever you'd use without the option"

> Then again, what's
> the difference between leaving this option out, and specifying
> "--with-thumb=toolchain"? Is it that we may override the toolchain defaults in
> configure, so specifying =toolchain means "don't do that"? That's kind of
> confusing, and I'm not sure that that distinction is going to be easy to convey
> to users.

It might be confusing, but I haven't found anything better considering the constraints, the main one being that the mobile people want flags used for official builds to be used by developers (which is sensible), yet, other people may want to override that behaviour and use whatever the toolchain defaults to, which is what =toolchain is about.

> @@ +6954,5 @@
> +    MOZ_THUMB=$withval)
> +
> +MOZ_ARG_WITH_STRING(thumb-interwork,
> +[  --with-thumb-interwork[[=yes|no|toolchain]]
> +                           Use Thumb/ARM instuctions interwork
> (-mthumb-interwork)],
> 
> This feels like it'd be better suited as a MOZ_ARG_ENABLE_BOOL, since the only
> options are enable/disable.

It's enable/disable/"whichever the toolchain defaults to, I don't want to force anything". For example, maemo and android default to softfp, but the user wanting to build may have a toolchain that defaults to hardfp and want to use that instead, yet may not want to have to update later when his toolchain changes to another default.
 
> @@ +6986,5 @@
> +    MOZ_FLOAT_ABI=$withval)
> +
> +MOZ_ARG_WITH_STRING(soft-float,
> +[  --with-soft-float[[=yes|no|toolchain]]
> +                           Use soft float library (-msoft-float)],
> 
> This seems like it should also be MOZ_ARG_ENABLE_BOOL.

Likewise.
Comment 67 Ted Mielczarek [:ted.mielczarek] 2011-05-02 07:40:34 PDT
As I said on IRC, perhaps "toolchain-default" would be a better string there. Verbose, but self-explanatory.

Given the yes/no/default bit, I suppose leaving those as --with is fine.
Comment 68 Mike Hommey [:glandium] 2011-05-02 08:11:15 PDT
Created attachment 529479 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

Updated per comment 67.
Comment 69 Mike Hommey [:glandium] 2011-05-02 08:13:54 PDT
Created attachment 529481 [details] [diff] [review]
Modify the way arm compiler flags are set in configure - NSPR part
Comment 70 Ted Mielczarek [:ted.mielczarek] 2011-05-03 11:17:04 PDT
Checked in the NSPR patch to NSPR CVS:
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.301; previous revision: 1.300
done
Comment 71 Ted Mielczarek [:ted.mielczarek] 2011-05-03 12:08:19 PDT
I tagged the tip of NSPR CVS with NSPR_HEAD_20110503 . rs=me to update the mozilla-central snapshot of NSPR with that tag.
Comment 72 Ted Mielczarek [:ted.mielczarek] 2011-05-03 12:18:33 PDT
Actually, I'm not sure. Hold off on updating NSPR till we sort this out.
Comment 73 Kai Engert (:kaie) 2011-05-04 06:46:35 PDT
Ted, I believe you forgot to update nsprpub/configure - Wan-Teh usually updates both files.

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.299; previous revision: 1.298
done
Comment 74 Kai Engert (:kaie) 2011-05-04 07:10:50 PDT
These patches don't seem to work.

Because of comment 73 you probably falsely assumed to have tested it.
After I updated configure, I got a build failure.

line 3210: syntax error near unexpected token `thumb2,' 
line 3210: `MOZ_ARG_DISABLE_BOOL(thumb2, , MOZ_THUMB=no, MOZ_THUMB=yes)'

Mike suggested a patch on top of that, which I'm attaching.
I allowed me to pass this failure,
but then I ran into the next one:

checking whether the chosen combination of compiler flags (-marm) works... configure: error: no
configure: error: /plaindata/moz/mocent/mozilla/nsprpub/configure failed for nsprpub

we've agreed to backout.
Comment 75 Kai Engert (:kaie) 2011-05-04 07:11:39 PDT
Created attachment 529993 [details] [diff] [review]
some intermediate patch
Comment 76 Kai Engert (:kaie) 2011-05-04 07:16:44 PDT
Backed out

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.300; previous revision: 1.299
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.303; previous revision: 1.302
done
Comment 77 Kai Engert (:kaie) 2011-05-04 07:18:21 PDT
I would like to propose to delay relanding fixes until after we have released 4.8.8, because we are trying to get a combined nspr 4.8.8 + nss 3.12.10 release done
Comment 78 Mike Hommey [:glandium] 2011-05-04 08:26:26 PDT
Created attachment 530022 [details] [diff] [review]
Modify the way arm compiler flags are set in configure - NSPR part

I don't know how I "tested" the previous patch... the problem was MOZ_ autoconf macros not being defined.

This time I think it's better if other eyes look at it. Same patch but with standard AC_ replacement macros.
Comment 79 Ted Mielczarek [:ted.mielczarek] 2011-05-06 06:27:58 PDT
Comment on attachment 530022 [details] [diff] [review]
Modify the way arm compiler flags are set in configure - NSPR part

Review of attachment 530022 [details] [diff] [review]:
-----------------------------------------------------------------

We'll land this after NSPR 4.8.8 is done.
Comment 80 Ted Mielczarek [:ted.mielczarek] 2011-05-11 17:14:16 PDT
*** Bug 656449 has been marked as a duplicate of this bug. ***
Comment 81 Oleg Romashin (:romaxa) 2011-05-11 17:21:06 PDT
is anything else preventing us from landing this patch?
Comment 82 Mike Hommey [:glandium] 2011-05-11 21:41:22 PDT
(In reply to comment #81)
> is anything else preventing us from landing this patch?

nspr
Comment 83 Kai Engert (:kaie) 2011-05-12 03:52:47 PDT
FYI, NSPR 4.8.8 has been released.

However, please make a tryserver build on all platforms, to make sure it works.
Comment 84 Mike Hommey [:glandium] 2011-05-13 01:42:40 PDT
Built successfully on try:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-942c55a90a10/

Note that the configure part of the nspr patch is actually wrong, it wasn't updated after the configure.in part. Unfortunately, I don't have the same version of autoconf as the one used in the nspr source, so running autoconf leads to a much more massive difference than it should. For some reason, nspr configure is not generated with the same autoconf version as the one for the main and js/src configures. So when applying the patch, please rerun your autoconf version and update the patch accordingly.
Comment 85 Mike Hommey [:glandium] 2011-05-26 23:44:12 PDT
When can we land this?
Comment 86 Ted Mielczarek [:ted.mielczarek] 2011-05-31 04:58:20 PDT
Crap, I didn't re-land this in CVS, did I? We just took a NSPR update on m-c, too. Bah! We should be able to get this in on trunk before the next migration to Aurora.
Comment 87 Stephanie Daugherty [:sdaugherty] 2011-06-01 02:02:41 PDT
Patch no longer cleanly applies to m-c trunk.
Comment 88 Mike Hommey [:glandium] 2011-06-01 03:19:26 PDT
Created attachment 536556 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

Refreshed against current m-c. Only context changes.
Comment 89 Mike Hommey [:glandium] 2011-06-01 03:21:07 PDT
Created attachment 536557 [details] [diff] [review]
Modify the way arm compiler flags are set in configure - NSPR part

Refreshed against current nsprpub as in m-c. Ran autoconf from redhat, which looks like what was used last on nspr, this time
Comment 90 Ted Mielczarek [:ted.mielczarek] 2011-06-01 07:02:15 PDT
Landed in NSPR CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.305; previous revision: 1.304
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.307; previous revision: 1.306
done

ping me and we'll coordinate tagging NSPR and updating it in m-c.
Comment 91 Ted Mielczarek [:ted.mielczarek] 2011-06-02 16:16:29 PDT
wtc: I tagged HEAD with NSPR_4_8_9_BETA3. The only change from beta 2 is this patch.
Comment 92 Wan-Teh Chang 2011-06-02 16:30:22 PDT
NSPR_4_8_9_BETA3 also contains a change to mozilla/nsprpub/pr/src/misc/prsystem.c
for bug 661351.

Why doesn't NSPR_4_8_9_BETA3 include mozilla/nsprpub/build/win32/pgomerge.py?
Comment 93 Ted Mielczarek [:ted.mielczarek] 2011-06-02 16:35:03 PDT
Ah, I was just going to mention that.

wtc: I re-updated properly and re-tagged. Apparently my checkout was out of date, and I forgot to "cvs up -d".

Note You need to log in before you can comment on or make changes to this bug.