Closed
Bug 873332
Opened 12 years ago
Closed 11 years ago
Don't use -fno-omit-frame-pointer for MOZ_PROFILING on ARM
Categories
(Firefox Build System :: General, defect)
Tracking
(b2g18?)
RESOLVED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
b2g18 | ? | --- |
People
(Reporter: jld, Assigned: jld)
References
Details
(Keywords: perf, Whiteboard: [c=profiling p=3])
Attachments
(1 file, 1 obsolete file)
The -fno-omit-frame-pointer option doesn't help us with stack unwinding — in Thumb mode, it sets r7 to an address that's not a constant offset from the saved registers we care about, so per-function information is still necessary to unwind. (Additionally, unlike on x86 where an rSP-based address can require an extra byte to encode, on Thumb it's no larger and may be smaller, as there are special extended-range immediate-offset instructions for stack access.) But, more importantly, it has a bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54398 I saw this result in miscompiling image::Decoder::NeedNewFrame such that mNewFrameData.nFrameNum was uninitialized (or, rather, initialized with stack garbage), preventing all image loading. The resulting phone may be faster due to the complete lack of image rendering, but is rather difficult to use due to lacking most of the UI. So, we shouldn't use -fno-omit-frame-pointer for MOZ_PROFILING on B2G, and possibly also Android. Finally, for the sake of completeness: the iOS ARM ABI differs from Android in this respect, and does have a frame pointer option that allows simple unwinding, but iOS is not currently a Gecko target.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
Hopefully I'm not missing any relevant instances of this flag. Tested by inspecting disassembly, and with my work-in-progress for bug 810526. Let me know if there are any try builds I ought to run.
Attachment #751284 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Component: Builds → Build Config
Product: Boot2Gecko → Core
Version: unspecified → Trunk
Assignee | ||
Updated•12 years ago
|
Attachment #751284 -
Flags: review? → review?(khuey)
Comment 2•12 years ago
|
||
Julian and I were just talking about this. This seems like the right thing to do.
Comment on attachment 751284 [details] [diff] [review] Remove -fno-omit-frame-pointer from profiling builds on ARM. Review of attachment 751284 [details] [diff] [review]: ----------------------------------------------------------------- I think glandium is more likely to know if there's any problem with doing this.
Attachment #751284 -
Flags: review?(khuey) → review?(mh+mozilla)
Comment 4•12 years ago
|
||
Comment on attachment 751284 [details] [diff] [review] Remove -fno-omit-frame-pointer from profiling builds on ARM. Review of attachment 751284 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/build/autoconf/frameptr.m4 @@ +14,5 @@ > esac > if test "$GNU_CC"; then > MOZ_ENABLE_FRAME_PTR="-fno-omit-frame-pointer $unwind_tables" > MOZ_DISABLE_FRAME_PTR="-fomit-frame-pointer" > + case "$target" in if test "$CPU_ARCH" = arm; then @@ +17,5 @@ > MOZ_DISABLE_FRAME_PTR="-fomit-frame-pointer" > + case "$target" in > + arm-*) > + # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54398 > + MOZ_ENABLE_FRAME_PTR="$unwind_tables" Note this is apparently fixed in newer versions of gcc, so maybe we'd want to do some version checking. However, considering the non-usefulness of the frame pointer anyways (breakpad can't handle interworking, and i don't think we even enable breakpad using it), we probably don't care.
Attachment #751284 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #4) > if test "$CPU_ARCH" = arm; then Done; thanks. > Note this is apparently fixed in newer versions of gcc, so maybe we'd want > to do some version checking. However, considering the non-usefulness of the > frame pointer anyways (breakpad can't handle interworking, and i don't think > we even enable breakpad using it), we probably don't care. The GCC bug (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54398) wasn't marked resolved, so I didn't realize that it was actually fixed. But I looked a little more closely: it was originally found on the aarch64 port[1], committed for 4.7 and up (r184169) and uplifted to 4.6 ([2], r191315). But I agree that we probably don't care. [1] http://gcc.gnu.org/ml/gcc-patches/2012-01/msg00201.html and http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00638.html [2] http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00844.html
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #751284 -
Attachment is obsolete: true
Attachment #753586 -
Flags: checkin?
Updated•11 years ago
|
Attachment #753586 -
Flags: checkin? → checkin+
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ad98a8e66316
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•