Closed Bug 577531 Opened 14 years ago Closed 14 years ago

Fix non-thumb2 builds on Android

Categories

(NSPR :: NSPR, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mwu, Assigned: blassey)

References

Details

(Keywords: regression)

Attachments

(4 files)

Attached patch FixSplinter Review
Bug 563751 regressed non-thumb2 builds and as a result, non-thumb2 builds don't work on the Android emulator. This patch restores the CFLAGS/CXXFLAGS that make it work.
Attachment #456474 - Flags: review?(ted.mielczarek)
Attachment #456474 - Attachment is patch: true
Attachment #456474 - Attachment mime type: application/octet-stream → text/plain
Attachment #456474 - Flags: review?(ted.mielczarek) → review+
Blocks: 567078
perhaps we should have these flags only for non-thumb builds
(In reply to comment #1)
> perhaps we should have these flags only for non-thumb builds
Maybe. -mthumb-interwork does increase code size, according to the gcc documentation. This patch just makes things consistent with the other configure.ins.
Keywords: checkin-needed
Attached patch patchSplinter Review
i'd prefer to fix it with something like this so we don't have -march=armv5te and -march=armv7 at the same time. Also -mthumb-interwork presumably produces larger, sightly slower code, so let's not use it if we don't have to.
Attachment #465298 - Flags: review?(ted.mielczarek)
(In reply to comment #4)
> Created attachment 465298 [details] [diff] [review]
> patch
> 
> i'd prefer to fix it with something like this so we don't have -march=armv5te
> and -march=armv7 at the same time.
That's not really a problem. Gcc takes the last one you specify, and we already do this everywhere else.

> Also -mthumb-interwork presumably produces
> larger, sightly slower code, so let's not use it if we don't have to.
This is somewhat risky I think. We have thumb-interwork on everywhere else. Have you tested with thumb-interwork turned off elsewhere in the code? I would want to make sure we can do this safely because I think the removal of this flag is what actually causes failure to run on the emulator. Maybe the system libraries use thumb..
(In reply to comment #5)
> > Also -mthumb-interwork presumably produces
> > larger, sightly slower code, so let's not use it if we don't have to.
> This is somewhat risky I think. We have thumb-interwork on everywhere else.
> Have you tested with thumb-interwork turned off elsewhere in the code? I would
> want to make sure we can do this safely because I think the removal of this
> flag is what actually causes failure to run on the emulator. Maybe the system
> libraries use thumb..

Hm, nevermind, I see that it's left on for armv5. If removing it elsewhere doesn't break things, lets do it.
With these patches I've built with thumb2 enabled and disabled and both run fine on the nexus one. The build with --disable-thumb2 runs normally on the emulator.
Blocks: 586772
Comment on attachment 465298 [details] [diff] [review]
patch

You'll need to split out the NSPR patch if you want me to land that for you.
Attachment #465298 - Flags: review?(ted.mielczarek) → review+
Attached patch nspr patchSplinter Review
Attachment #468624 - Flags: review+
Attached patch m-c patchSplinter Review
Attachment #468625 - Flags: review+
Assignee: mwu → blassey.bugs
Landed the NSPR patch in NSPR CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.287; previous revision: 1.286
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.291; previous revision: 1.290
done
Attachment #468625 - Flags: approval2.0?
Comment on attachment 468625 [details] [diff] [review]
m-c patch

make sure this passed try.
Attachment #468625 - Flags: approval2.0? → approval2.0+
pushed http://hg.mozilla.org/mozilla-central/rev/a2e5d3cbf6cf
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 616413
Depends on: 620239
Depends on: 618789
Target Milestone: --- → 4.8.7
You need to log in before you can comment on or make changes to this bug.