Closed
Bug 626035
Opened 13 years ago
Closed 13 years ago
-march=armv5+ -mthumb* forced on all arm platforms
Categories
(NSPR :: NSPR, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
4.8.9
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?
Assignee | ||
Updated•13 years ago
|
Attachment #504094 -
Flags: review? → review?(wtc)
Assignee | ||
Updated•13 years ago
|
Attachment #504094 -
Attachment is patch: true
Attachment #504094 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 1•13 years ago
|
||
Same patch, for m-c
Assignee: wtc → mh+mozilla
Attachment #504095 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 2•13 years ago
|
||
This is great, bug 618789 made it even worse.
Assignee | ||
Comment 3•13 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•13 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•13 years ago
|
||
blassey: ping!
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 4.8.8
Comment 6•13 years ago
|
||
Mike, is this bug a duplicate of bug 616413?
Assignee | ||
Comment 7•13 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 8•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 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.
Comment 12•13 years ago
|
||
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•13 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•13 years ago
|
||
(In reply to comment #13) > why romaxa added them Actually, that was blassey, in bug 563751
Assignee | ||
Comment 15•13 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•13 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
Comment 17•13 years ago
|
||
(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•13 years ago
|
||
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 19•13 years ago
|
||
Comment 20•13 years ago
|
||
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•13 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.
Comment 22•13 years ago
|
||
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•13 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.
Comment 24•13 years ago
|
||
tested without setting - ac_cv_gcc_arm_thumb2="no", and it also works fine.
Updated•13 years ago
|
Attachment #511322 -
Flags: review? → review?(mh+mozilla)
Assignee | ||
Comment 25•13 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•13 years ago
|
||
Note we should do the same in nsprpub, too.
Assignee | ||
Comment 27•13 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)
Comment 29•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #507806 -
Flags: feedback?
Updated•13 years ago
|
Attachment #508348 -
Flags: feedback?
Assignee | ||
Comment 30•13 years ago
|
||
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•13 years ago
|
||
Apparently, removing the whole thing in nspr should be enough.
Attachment #516548 -
Flags: review?(wtc)
Updated•13 years ago
|
OS: Linux → Windows CE
Assignee | ||
Updated•13 years ago
|
OS: Windows CE → All
Updated•13 years ago
|
OS: All → Linux
Comment 32•13 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•13 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•13 years ago
|
||
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•13 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•13 years ago
|
OS: Windows CE → Linux
Assignee | ||
Comment 36•13 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?
Comment 37•13 years ago
|
||
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•13 years ago
|
||
If Fennec RC going to merge nspr 4.8.8 ?
Comment 39•13 years ago
|
||
that's the plan (see bug 639191)
Assignee | ||
Comment 40•13 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•13 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•13 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•13 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•13 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•13 years ago
|
Attachment #504094 -
Attachment is obsolete: false
Comment 45•13 years ago
|
||
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 47•13 years ago
|
||
Any chance to get this patch landed?
Assignee | ||
Comment 48•13 years ago
|
||
blassey doesn't want it.
Assignee | ||
Comment 49•13 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•13 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?
Comment 51•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
Attachment #526726 -
Flags: feedback?(romaxa)
Assignee | ||
Comment 53•13 years ago
|
||
Comment 54•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #526726 -
Flags: review?(blassey.bugs) → review?(ted.mielczarek)
Comment 55•13 years ago
|
||
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•13 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 57•13 years ago
|
||
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•13 years ago
|
||
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•13 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 60•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 years ago
|
||
Attachment #526727 -
Attachment is obsolete: true
Attachment #526942 -
Flags: review?(wtc)
Assignee | ||
Comment 64•13 years ago
|
||
Attachment #526941 -
Attachment is obsolete: true
Attachment #528540 -
Flags: review?(ted.mielczarek)
Attachment #526941 -
Flags: review?(ted.mielczarek)
Comment 65•13 years ago
|
||
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•13 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.
Comment 67•13 years ago
|
||
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•13 years ago
|
||
Updated per comment 67.
Assignee | ||
Updated•13 years ago
|
Attachment #529479 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Attachment #528540 -
Attachment is obsolete: true
Assignee | ||
Comment 69•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #526942 -
Attachment is obsolete: true
Attachment #526942 -
Flags: review?(wtc)
Comment 70•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Actually, I'm not sure. Hold off on updating NSPR till we sort this out.
Comment 73•13 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•13 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•13 years ago
|
||
Comment 76•13 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•13 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•13 years ago
|
||
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•13 years ago
|
Attachment #529481 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #529993 -
Attachment is obsolete: true
Updated•13 years ago
|
Target Milestone: 4.8.8 → 4.8.9
Comment 79•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #504094 -
Attachment is obsolete: false
Comment 81•13 years ago
|
||
is anything else preventing us from landing this patch?
Assignee | ||
Comment 82•13 years ago
|
||
(In reply to comment #81) > is anything else preventing us from landing this patch? nspr
Comment 83•13 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•13 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•13 years ago
|
||
When can we land this?
Comment 86•13 years ago
|
||
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•13 years ago
|
||
Patch no longer cleanly applies to m-c trunk.
Assignee | ||
Comment 88•13 years ago
|
||
Refreshed against current m-c. Only context changes.
Assignee | ||
Updated•13 years ago
|
Attachment #529479 -
Attachment is obsolete: true
Assignee | ||
Comment 89•13 years ago
|
||
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•13 years ago
|
Attachment #530022 -
Attachment is obsolete: true
Comment 90•13 years ago
|
||
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•13 years ago
|
||
wtc: I tagged HEAD with NSPR_4_8_9_BETA3. The only change from beta 2 is this patch.
Comment 92•13 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?
Comment 93•13 years ago
|
||
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•13 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•