Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Description • 11 years ago

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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 1 • 11 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.

	(not currently active) Ted Mielczarek
	Comment 2 • 11 years ago

Julian and I were just talking about this. This seems like the right thing to do.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 3 • 11 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]:
-----------------------------------------------------------------

I think glandium is more likely to know if there's any problem with doing this.

	Mike Hommey [:glandium]
	Comment 4 • 11 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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 5 • 11 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

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad98a8e66316

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/ad98a8e66316