863264 - Build Android Nightly to support Breakpad Unwinding

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Julian Seward [:jseward]]

Android nightlies don't support Breakpad unwinding.  We need to
enable that.

Comment 1

[Julian Seward [:jseward]]

Attachment: Patch

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 739059 [details] [diff] [review]
Patch

I suspect the only reason this is missing is because prior to bug 732162 this didn't actually do anything useful for Android builds.

Do we want to add this to the armv6/x86 mozconfigs as well?

Comment 3

[(no longer active)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> Comment on attachment 739059 [details] [diff] [review]
> Patch
> 
> I suspect the only reason this is missing is because prior to bug 732162
> this didn't actually do anything useful for Android builds.

Yep.

Comment 4

[(no longer active)]

Also, remember that this will turn on the Start Profiling menu entry IIRC.  Please check with the mobile team before landing this.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Also, remember that this will turn on the Start Profiling menu entry IIRC. 
> Please check with the mobile team before landing this.

Last I checked that menu doesn't compile anymore. We should just remove it.

Comment 6

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #5)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> > Also, remember that this will turn on the Start Profiling menu entry IIRC. 
> > Please check with the mobile team before landing this.
> 
> Last I checked that menu doesn't compile anymore. We should just remove it.

It does compile but since the menu aren't useful on their own (they only toggle the profiler) I think we should remove them. Filed bug 863375.

Comment 7

[(no longer active)]

(In reply to comment #6)
> (In reply to Benoit Girard (:BenWa) from comment #5)
> > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> > > Also, remember that this will turn on the Start Profiling menu entry IIRC. 
> > > Please check with the mobile team before landing this.
> > 
> > Last I checked that menu doesn't compile anymore. We should just remove it.
> 
> It does compile but since the menu aren't useful on their own (they only toggle
> the profiler) I think we should remove them. Filed bug 863375.

OK, the reason I said this is that last time that something random showed that menu on Nightly builds people complained...

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's land this together with bug 863375, to minimize any UI issues.

Comment 9

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c9af867443c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed9d6cd69565

Comment 11

[Julian Seward [:jseward]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> Backed out for Android reftest failures.

This appears to have been caused, at least in part, by a gcc
bug.  This is with the gcc-4.6.2 variant in ndk-r8c.

Bug happens at -O2 -fno-omit-frame-pointer.  The addition of
-fno-omit-frame-pointer is a result of --enable-profiling and hence
that's why it fails when it didn't fail before.

Bug does not happen at -O -fno-omit-frame-pointer (as far as I can
tell).

The problem is miscompilation of gfxFont::RunMetrics::RunMetrics in
gfx/thebes/gfxFont.h.  This should be compiled into code that zeroes
out 7 'double' fields in *this, but it also appears to copy 16 bytes
of uninitialised stack to the start of *this.  The test case
layout/reftests/abs-pos/table-3.html fails intermittently, and
valgrind complains a lot about uninitialised values originating from
stack allocation in RunMetrics().

gfxFont::RunMetrics::RunMetrics looks like this:

        RunMetrics() {
            mAdvanceWidth = mAscent = mDescent = 0.0;
            mBoundingBox = gfxRect(0,0,0,0);
        }

Nathan Froyd points out that the assignment to mBoundingBox is
probably redundant since that's what the default initialiser will do
anyway.  Commenting out that assignment produces assembly code which
looks correct, shows no failures on table-3.html, and gets no
complaints from valgrind.

I am not sure how to proceed.  Options are:

* remove the mBoundingBox assignment, to work around the compiler bug

* disable frame pointers?  We might be able to get by without them if
  we have a good enough CFI and EXIDX story on ARM.

* both of the above?

Comment 12

[Julian Seward [:jseward]]

FTR, the bogus assembly for RunMetrics() is this:

00ccb7d4 <_ZN7gfxFont10RunMetricsC1Ev>:
  ccb7d4:       b5f0            push    {r4, r5, r6, r7, lr}
  ccb7d6:       b089            sub     sp, #36 ; 0x24
  ccb7d8:       af00            add     r7, sp, #0
  ccb7da:       4604            mov     r4, r0
  ccb7dc:       2200            movs    r2, #0
  ccb7de:       463e            mov     r6, r7
  ccb7e0:       2300            movs    r3, #0
  ccb7e2:       4605            mov     r5, r0
  ccb7e4:       e9c0 230a       strd    r2, r3, [r0, #40]       ; 0x28
  ccb7e8:       e9c0 230c       strd    r2, r3, [r0, #48]       ; 0x30
  ccb7ec:       e9c0 2304       strd    r2, r3, [r0, #16]
  ccb7f0:       e9c0 2302       strd    r2, r3, [r0, #8]
  ccb7f4:       e9c7 2304       strd    r2, r3, [r7, #16]
  ccb7f8:       e8e4 2306       strd    r2, r3, [r4], #24
  ccb7fc:       e9c7 2306       strd    r2, r3, [r7, #24]
  ccb800:       ce0f            ldmia   r6!, {r0, r1, r2, r3}
  ccb802:       c40f            stmia   r4!, {r0, r1, r2, r3}
  ccb804:       e896 000f       ldmia.w r6, {r0, r1, r2, r3}
  ccb808:       e884 000f       stmia.w r4, {r0, r1, r2, r3}
  ccb80c:       4628            mov     r0, r5
  ccb80e:       f107 0724       add.w   r7, r7, #36     ; 0x24
  ccb812:       46bd            mov     sp, r7
  ccb814:       bdf0            pop     {r4, r5, r6, r7, pc}

Comment 13

[(not currently active) Ted Mielczarek]

If the code change is not invasive and GFX peers are ok with it, then that sounds like the simplest solution.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: delete pointless initialization of mBoundingBox in gfxFont::RunMetrics constructor

Comment 15

[Benoit Girard (:BenWa)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> If the code change is not invasive and GFX peers are ok with it, then that
> sounds like the simplest solution.

The change is fine but...

Would be really start building and shipping profiling nightlies with a configuration that is so flacky something like that fails?

Comment 16

[(not currently active) Ted Mielczarek]

We work around compiler bugs all the time. I don't think this one is any more special than any other. If we can reduce a testcase we can probably find out if it's fixed in a newer GCC, and if not get it filed upstream.

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 743239 [details] [diff] [review]
delete pointless initialization of mBoundingBox in gfxFont::RunMetrics constructor

r+ per ted's comment

Comment 18

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1b078e8bf5

Leaving open so Julian can repush his patch.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7a1b078e8bf5

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> We work around compiler bugs all the time. I don't think this one is any
> more special than any other. If we can reduce a testcase we can probably
> find out if it's fixed in a newer GCC, and if not get it filed upstream.

A *really* cut down testcase seems to work just fine in a pre-release version of 4.7 and a HEAD-ish 4.9 (both targeting arm-eabi) compiler that I have lying around.  It generates code identical to what one would expect.  I don't know the exact compilation options that were used, so I guessed and played around with several variations.  And it's possible that the more complicated C++ used in gfxFont.h is what caused problems; I'd be happy to experiment if Julian provided me with preprocessed source and the compilation options for gfxFont.cpp.

But I'd be willing to bet that this issue is fixed.

Comment 21

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/709034aa6a03

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Does -fno-omit-frame-pointer accomplish anything useful here?  As far as I can tell from compiling on B2G, it just makes the code less efficient and doesn't remove the requirement to intepret per-PC-range unwinding information (as it would on x86 and, apparently, on iOS).

Comment 23

[Julian Seward [:jseward]]

(In reply to Jed Davis [:jld] from comment #22)
> Does -fno-omit-frame-pointer accomplish anything useful here?

I suspect it doesn't, but I can't say that for sure.  Bug 863705
added stats gathering that shows how often Breakpad uses each of
its various schemes (CFI, FP chasing, stack scan), so some experimenting
with that would tell us.

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/709034aa6a03