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

RESOLVED FIXED in 4.8.9

Status

NSPR
NSPR
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

4.8.7
4.8.9
ARM
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 17 obsolete attachments)

445 bytes, patch
Wan-Teh Chang
: review+
blassey
: review+
Details | Diff | Splinter Review
23.75 KB, patch
Details | Diff | Splinter Review
72.74 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
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.
Attachment #504094 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #504094 - Flags: review? → review?(wtc)
(Assignee)

Updated

7 years ago
Attachment #504094 - Attachment is patch: true
Attachment #504094 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 1

7 years ago
Created attachment 504095 [details] [diff] [review]
Only add  -march=armv5te -mthumb-interwork on android (for m-c)

Same patch, for m-c
Assignee: wtc → mh+mozilla
Attachment #504095 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 2

7 years ago
This is great, bug 618789 made it even worse.
(Assignee)

Comment 3

7 years ago
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

7 years ago
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+

Comment 5

7 years ago
blassey: ping!
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 4.8.8

Comment 6

7 years ago
Mike, is this bug a duplicate of bug 616413?
(Assignee)

Comment 7

7 years ago
(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 9

7 years ago
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)
(Assignee)

Comment 10

7 years ago
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.
Attachment #504094 - Attachment is obsolete: true
Attachment #504095 - Attachment is obsolete: true
Attachment #507806 - Flags: feedback?
Attachment #504095 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 11

7 years ago
(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.
(Assignee)

Comment 13

7 years ago
(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.
(Assignee)

Comment 14

7 years ago
(In reply to comment #13)
> why romaxa added them

Actually, that was blassey, in bug 563751
(Assignee)

Comment 15

7 years ago
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.
(Assignee)

Comment 16

7 years ago
(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
(Assignee)

Comment 18

7 years ago
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.
Attachment #508348 - Flags: feedback?
Created attachment 511151 [details] [diff] [review]
Updated version of alternative patch, extended to js
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...
(Assignee)

Comment 21

7 years ago
(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.
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
Attachment #511151 - Attachment is obsolete: true
Attachment #511322 - Flags: review?
(Assignee)

Comment 23

7 years ago
(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)
(Assignee)

Comment 25

7 years ago
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)
(Assignee)

Comment 26

7 years ago
Note we should do the same in nsprpub, too.
(Assignee)

Comment 27

7 years ago
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+
Attachment #507806 - Flags: feedback?
Attachment #508348 - Flags: feedback?
(Assignee)

Updated

6 years ago
Depends on: 636701
(Assignee)

Comment 30

6 years ago
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+
Attachment #507806 - Attachment is obsolete: true
Attachment #508348 - Attachment is obsolete: true
Attachment #511322 - Attachment is obsolete: true
Attachment #516547 - Flags: review+
(Assignee)

Comment 31

6 years ago
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.
Attachment #516548 - Flags: review?(wtc)

Updated

6 years ago
OS: Linux → Windows CE
(Assignee)

Updated

6 years ago
OS: Windows CE → All

Updated

6 years ago

Comment 32

6 years ago
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+
(Assignee)

Comment 33

6 years ago
(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

6 years ago
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
Attachment #516548 - Attachment is obsolete: true

Comment 35

6 years ago
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

Updated

6 years ago
OS: Windows CE → Linux
(Assignee)

Comment 36

6 years ago
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.
(Assignee)

Comment 38

6 years ago
If Fennec RC going to merge nspr 4.8.8 ?
that's the plan (see bug 639191)
(Assignee)

Comment 40

6 years ago
(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.
(Assignee)

Comment 41

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 43

6 years ago
(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

6 years ago
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)

Updated

6 years ago
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:
Duplicate of this bug: 632915
(Assignee)

Updated

6 years ago
Blocks: 646846
Any chance to get this patch landed?
(Assignee)

Comment 48

6 years ago
blassey doesn't want it.
(Assignee)

Comment 49

6 years ago
(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
(Assignee)

Comment 50

6 years ago
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
(Assignee)

Comment 52

6 years ago
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.
Attachment #504094 - Attachment is obsolete: true
Attachment #516547 - Attachment is obsolete: true
Attachment #517747 - Attachment is obsolete: true
Attachment #526726 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Attachment #526726 - Flags: feedback?(romaxa)
(Assignee)

Comment 53

6 years ago
Created attachment 526727 [details] [diff] [review]
Modify the way arm compiler flags are set in configure. nspr part
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
(Assignee)

Comment 56

6 years ago
(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.

Comment 59

6 years ago
(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+
(Assignee)

Comment 61

6 years ago
(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.
(Assignee)

Comment 62

6 years ago
Created attachment 526941 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

With the thumb-interwork typo fixed.
Attachment #526726 - Attachment is obsolete: true
Attachment #526941 - Flags: review?(ted.mielczarek)
Attachment #526726 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 63

6 years ago
Created attachment 526942 [details] [diff] [review]
Modify the way arm compiler flags are set in configure. nspr part
Attachment #526727 - Attachment is obsolete: true
Attachment #526942 - Flags: review?(wtc)
(Assignee)

Comment 64

6 years ago
Created attachment 528540 [details] [diff] [review]
Modify the way arm compiler flags are set in configure
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+
(Assignee)

Comment 66

6 years ago
(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.
(Assignee)

Comment 68

6 years ago
Created attachment 529479 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

Updated per comment 67.
(Assignee)

Updated

6 years ago
Attachment #529479 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #528540 - Attachment is obsolete: true
(Assignee)

Comment 69

6 years ago
Created attachment 529481 [details] [diff] [review]
Modify the way arm compiler flags are set in configure - NSPR part
(Assignee)

Updated

6 years ago
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.

Comment 73

6 years ago
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

6 years ago
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

6 years ago
Created attachment 529993 [details] [diff] [review]
some intermediate patch

Comment 76

6 years ago
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

6 years ago
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
(Assignee)

Comment 78

6 years ago
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.
Attachment #530022 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

6 years ago
Attachment #529481 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #529993 - Attachment is obsolete: true

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
Blocks: 547946

Updated

6 years ago
Attachment #504094 - Attachment is obsolete: false
Duplicate of this bug: 656449
is anything else preventing us from landing this patch?
(Assignee)

Comment 82

6 years ago
(In reply to comment #81)
> is anything else preventing us from landing this patch?

nspr

Comment 83

6 years ago
FYI, NSPR 4.8.8 has been released.

However, please make a tryserver build on all platforms, to make sure it works.
(Assignee)

Comment 84

6 years ago
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.
(Assignee)

Comment 85

6 years ago
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.
(Assignee)

Comment 88

6 years ago
Created attachment 536556 [details] [diff] [review]
Modify the way arm compiler flags are set in configure

Refreshed against current m-c. Only context changes.
(Assignee)

Updated

6 years ago
Attachment #529479 - Attachment is obsolete: true
(Assignee)

Comment 89

6 years ago
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
(Assignee)

Updated

6 years ago
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.

Comment 92

6 years ago
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".
(Assignee)

Comment 94

6 years ago
http://hg.mozilla.org/mozilla-central/rev/6bb5f12c636f
http://hg.mozilla.org/mozilla-central/rev/09aa6c7e5867
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Depends on: 674837
You need to log in before you can comment on or make changes to this bug.