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?

(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.

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.

(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.

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

(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?

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.

(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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1b078e8bf5 Leaving open so Julian can repush his patch.

(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.

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).

(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.