Closed Bug 626035 Opened 13 years ago Closed 13 years ago

-march=armv5+ -mthumb* forced on all arm platforms

Categories

(NSPR :: NSPR, defect, P1)

4.8.7
ARM
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(3 files, 17 obsolete files)

445 bytes, patch
wtc
: review+
blassey
: review+
Details | Diff | Splinter Review
23.75 KB, patch
Details | Diff | Splinter Review
72.74 KB, patch
Details | Diff | Splinter Review
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.
Attachment #504094 - Flags: review?
Attachment #504094 - Flags: review? → review?(wtc)
Attachment #504094 - Attachment is patch: true
Attachment #504094 - Attachment mime type: application/octet-stream → text/plain
Same patch, for m-c
Assignee: wtc → mh+mozilla
Attachment #504095 - Flags: review?(ted.mielczarek)
This is great, bug 618789 made it even worse.
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 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.
Attachment #504094 - Flags: review?(wtc)
Attachment #504094 - Flags: review?(blassey.bugs)
Attachment #504094 - Flags: review+
blassey: ping!
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 4.8.8
Mike, is this bug a duplicate of bug 616413?
(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 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.
Attachment #504094 - Flags: review?(blassey.bugs) → review+
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
Attachment #504094 - Attachment description: Only add -march=armv5te -mthumb-interwork on android → Only add -march=armv5te -mthumb-interwork on android (checked in to NSPR CVS trunk)
Attached patch New proposal (obsolete) — Splinter Review
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.
Attachment #504094 - Attachment is obsolete: true
Attachment #504095 - Attachment is obsolete: true
Attachment #507806 - Flags: feedback?
Attachment #504095 - Flags: review?(ted.mielczarek)
(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.
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.
(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.
(In reply to comment #13)
> why romaxa added them

Actually, that was blassey, in bug 563751
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.
(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
(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
Attached patch Alternative proposal (obsolete) — Splinter Review
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.
Attachment #508348 - Flags: feedback?
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...
(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.
Attached patch Updated, tested and compiled. (obsolete) — Splinter Review
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
Attachment #511151 - Attachment is obsolete: true
Attachment #511322 - Flags: review?
(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.
tested without setting - ac_cv_gcc_arm_thumb2="no", and it also works fine.
Attachment #511322 - Flags: review? → review?(mh+mozilla)
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%...
Attachment #511322 - Flags: review?(mh+mozilla) → review?(khuey)
Note we should do the same in nsprpub, too.
And before getting this to land, we need to have build people update mozconfigs for arm builds.
Comment on attachment 511322 [details] [diff] [review]
Updated, tested and compiled.

Punting
Attachment #511322 - Flags: review?(khuey) → review?(ted.mielczarek)
Blocks: 632915
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.
Attachment #511322 - Flags: review?(ted.mielczarek) → review+
Depends on: 636701
Same patch, removing the setting of MOZ_ARM_VFP_FLAGS which is now unused. Carrying over r+
Attachment #507806 - Attachment is obsolete: true
Attachment #508348 - Attachment is obsolete: true
Attachment #511322 - Attachment is obsolete: true
Attachment #516547 - Flags: review+
Apparently, removing the whole thing in nspr should be enough.
Attachment #516548 - Flags: review?(wtc)
OS: Linux → Windows CE
OS: Windows CE → All
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?
Attachment #516548 - Flags: review?(wtc) → review+
(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.
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
Attachment #516548 - Attachment is obsolete: true
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.
OS: Linux → Windows CE
OS: Windows CE → Linux
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?
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.
If Fennec RC going to merge nspr 4.8.8 ?
that's the plan (see bug 639191)
(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.
(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.
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.
(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 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
Attachment #517747 - Attachment description: Use toolchain defaults on ARM and let users set their own flags through environment, nspr part (as checked in) → Use toolchain defaults on ARM and let users set their own flags through environment, nspr part (checked in, then backed out)
Attachment #504094 - Attachment is obsolete: false
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:
Blocks: 646846
Any chance to get this patch landed?
blassey doesn't want it.
(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
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?
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
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.
Attachment #504094 - Attachment is obsolete: true
Attachment #516547 - Attachment is obsolete: true
Attachment #517747 - Attachment is obsolete: true
Attachment #526726 - Flags: review?(blassey.bugs)
Attachment #526726 - Flags: feedback?(romaxa)
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
Attachment #526726 - Flags: feedback+
Attachment #526726 - Flags: review?(blassey.bugs) → review?(ted.mielczarek)
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
(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 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 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.
(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 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
Attachment #526726 - Flags: feedback?(romaxa) → feedback+
(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.
With the thumb-interwork typo fixed.
Attachment #526726 - Attachment is obsolete: true
Attachment #526941 - Flags: review?(ted.mielczarek)
Attachment #526726 - Flags: review?(ted.mielczarek)
Attachment #526727 - Attachment is obsolete: true
Attachment #526942 - Flags: review?(wtc)
Attachment #526941 - Attachment is obsolete: true
Attachment #528540 - Flags: review?(ted.mielczarek)
Attachment #526941 - Flags: review?(ted.mielczarek)
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.
Attachment #528540 - Flags: review?(ted.mielczarek) → review+
(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.
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.
Attachment #529479 - Flags: review+
Attachment #528540 - Attachment is obsolete: true
Attachment #526942 - Attachment is obsolete: true
Attachment #526942 - Flags: review?(wtc)
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
I tagged the tip of NSPR CVS with NSPR_HEAD_20110503 . rs=me to update the mozilla-central snapshot of NSPR with that tag.
Actually, I'm not sure. Hold off on updating NSPR till we sort this out.
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
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.
Attached patch some intermediate patch (obsolete) — Splinter Review
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
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
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.
Attachment #530022 - Flags: review?(ted.mielczarek)
Attachment #529481 - Attachment is obsolete: true
Attachment #529993 - Attachment is obsolete: true
Target Milestone: 4.8.8 → 4.8.9
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.
Attachment #530022 - Flags: review?(ted.mielczarek) → review+
Blocks: 547946
Attachment #504094 - Attachment is obsolete: false
is anything else preventing us from landing this patch?
(In reply to comment #81)
> is anything else preventing us from landing this patch?

nspr
FYI, NSPR 4.8.8 has been released.

However, please make a tryserver build on all platforms, to make sure it works.
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.
When can we land this?
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.
Patch no longer cleanly applies to m-c trunk.
Refreshed against current m-c. Only context changes.
Attachment #529479 - Attachment is obsolete: true
Refreshed against current nsprpub as in m-c. Ran autoconf from redhat, which looks like what was used last on nspr, this time
Attachment #530022 - Attachment is obsolete: true
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.
wtc: I tagged HEAD with NSPR_4_8_9_BETA3. The only change from beta 2 is this patch.
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?
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".
http://hg.mozilla.org/mozilla-central/rev/6bb5f12c636f
http://hg.mozilla.org/mozilla-central/rev/09aa6c7e5867
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 674837
You need to log in before you can comment on or make changes to this bug.