Closed Bug 873332 Opened 9 years ago Closed 9 years ago

Don't use -fno-omit-frame-pointer for MOZ_PROFILING on ARM

Categories

(Firefox Build System :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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.
Blocks: 810526
tracking-b2g18: --- → ?
Whiteboard: c=profiling
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?
Component: Builds → Build Config
Product: Boot2Gecko → Core
Version: unspecified → Trunk
Attachment #751284 - Flags: review? → review?(khuey)
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 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+
(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
Attachment #753586 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/ad98a8e66316
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Keywords: perf
Whiteboard: c=profiling → [c=profiling p=3]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.