Closed
Bug 863264
Opened 12 years ago
Closed 12 years ago
Build Android Nightly to support Breakpad Unwinding
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: jseward, Assigned: jseward)
References
Details
Attachments
(2 files)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
Android nightlies don't support Breakpad unwinding. We need to enable that.
Assignee | ||
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
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+
Updated•12 years ago
|
Assignee: nobody → jseward
Comment 3•12 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•12 years ago
|
||
Also, remember that this will turn on the Start Profiling menu entry IIRC. Please check with the mobile team before landing this.
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
(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•12 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...
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c9af867443c
Comment 10•12 years ago
|
||
Backed out for Android reftest failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/ed9d6cd69565
Assignee | ||
Comment 11•12 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•12 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}
Comment 13•12 years ago
|
||
If the code change is not invasive and GFX peers are ok with it, then that sounds like the simplest solution.
Comment 15•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
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+
![]() |
||
Comment 18•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1b078e8bf5 Leaving open so Julian can repush his patch.
Whiteboard: [leave open]
![]() |
||
Comment 20•12 years ago
|
||
(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.
Assignee | ||
Comment 21•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/709034aa6a03
Comment 22•12 years ago
|
||
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•12 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•12 years ago
|
Whiteboard: [leave open]
Comment 24•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/709034aa6a03
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•