Build Android Nightly to support Breakpad Unwinding

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

Trunk
mozilla23
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Android nightlies don't support Breakpad unwinding.  We need to
enable that.
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?
Attachment #739059 - Flags: review+
Assignee: nobody → jseward

Comment 3

6 years ago
(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

6 years ago
Also, remember that this will turn on the Start Profiling menu entry IIRC.  Please check with the mobile team before landing this.
(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.
(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

6 years ago
(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...
Let's land this together with bug 863375, to minimize any UI issues.

Updated

6 years ago
Blocks: 863453
(Assignee)

Comment 11

6 years ago
(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?
(Assignee)

Comment 12

6 years ago
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}
If the code change is not invasive and GFX peers are ok with it, then that sounds like the simplest solution.
Created attachment 743239 [details] [diff] [review]
delete pointless initialization of mBoundingBox in gfxFont::RunMetrics constructor
Attachment #743239 - Flags: review?(bgirard)
(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?
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 on attachment 743239 [details] [diff] [review]
delete pointless initialization of mBoundingBox in gfxFont::RunMetrics constructor

r+ per ted's comment
Attachment #743239 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1b078e8bf5

Leaving open so Julian can repush his patch.
Whiteboard: [leave open]
(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.

Updated

6 years ago
Blocks: 867721
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).
(Assignee)

Comment 23

6 years ago
(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.
(Assignee)

Updated

6 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/709034aa6a03
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.