666918 - ARM builds fail with --enable-profiling

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jacob Bramley [:jbramley]]

Attachment: Override no-omit-frame-pointer for one NEON routine.

The --enable-profiling switch adds -fno-omit-frame-pointer to the compiler flags so that call graphs can be produced by profilers. However, if GCC is forced into maintaining a frame pointer, it does not have enough registers left over to satisfy the inline assembly contraints in yuv42x_to_rgb565_row_neon.

I've attached a trivial fix, which simply forces the frame pointer off just for that function. An arguably cleaner fix would edit the assembly contraints so that there are enough registers anyway, though this might not be straightforward and could well come with a performance penalty.

Comment 1

[Jacob Bramley [:jbramley]]

It looks like breakpad also has trouble. I'm trying a build with --disable-crashreporter. (I don't need the crash reporter for this.)

Comment 2

[Jacob Bramley [:jbramley]]

I think the crash reporter doesn't build because the Linux syscall interface uses r7, and GCC uses r7 as a frame pointer in Thumb-2 mode. I'm not sure exactly which asm statement is causing offence because the error I get doesn't point anywhere useful. There are many that use r7.

Most-likely solutions:
  * Force -fomit-frame-pointer for this module. We can't generally get stack traces into the kernel anyway, so not much is lost.
  * Force this module to build as ARM code, where the frame pointer is r11.

The latter is probably the cleanest solution. Thumb code is smaller so it'd cost some space, but I assume we don't set --enable-profiling for release builds anyway. (Yes, it could affect profile results, but it wouldn't be significant and it should be restricted to the crash reporter.)

This does not show up in NDK5, presumably because Thumb support is not detected (as in bug 667130) and so the module is being built with ARM support.

Comment 3

[Jacob Bramley [:jbramley]]

Ok, there's already a fix in the Makefile.in for exception_handler.cc, and -fomit-frame-pointer gets added to the command line. However, the profiling flags (-fno-omit-frame-pointer) get added afterwards, and counter-act it.

Comment 4

[Jacob Bramley [:jbramley]]

Attachment: Force omit-frame-pointer for some selected routines.

This patch forces omit-frame-pointer for the failing breakpad file. The only change to the existing workaround (from bug 633436) is to remove -fno-omit-frame-pointer from MOZ_OPTIMIZE_FLAGS.

Comment 5

[Jacob Bramley [:jbramley]]

Comment on attachment 542823 [details] [diff] [review]
Force omit-frame-pointer for some selected routines.

Review of attachment 542823 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/ycbcr/yuv_convert_arm.cpp
@@ +21,5 @@
> +                              const uint8 *y,
> +                              const uint8 *u,
> +                              const uint8 *v,
> +                              int n,
> +                              int oddflag)

By the way, I did wonder if this should use the same Makefile-based mechanism as for the breakpad fix, but decided that per-function controls would be better here. If we add other NEON functions to this file, it might be handy to control omit-frame-pointer on a per-function basis rather than for the whole file.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 542823 [details] [diff] [review]
Force omit-frame-pointer for some selected routines.

Per-function here is fine with me.

Comment 7

[Jacob Bramley [:jbramley]]

To where should I push this? mozilla-central?

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #7)
> To where should I push this? mozilla-central?

yes, or mozilla-incoming if you don't want to watch the tree

Comment 9

[Jacob Bramley [:jbramley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3d12cfc60979

Comment 10

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/3d12cfc60979