bug 1360322 turns on gecko profiler for aarch64, but lul doesn't.

Julian, ping. This issue depends on symbol resolution support on Android's gecko profiler (bug 1350500). To land bug 1350500, this fix is required.

Comment on attachment 8967642 [details] Bug 1450185 - Implement DWARF stack walker for aarch64. https://reviewboard.mozilla.org/r/236372/#review244544 Thank you for doing this! Basically it looks good. r+ if you fix the 3 comments below. ::: commit-message-da809:9 (Diff revision 1) > +not first frame. > + > +I test by gtest on Linux/aarch64, and profiler works on Android/aarch64. > + > +EM_AARCH64 might not be defined on our builders since headers are old, so > +this define is needed. And mAdminThreadId is unused on release build and Regarding mAdminThreadId, I see what the problem is (yes, it's unused on release builds). But I don't feel happy about the "Unused << mAdminThreadId" fix. Please remove that change. Clearly this still needs to be fixed, otherwise the warnings will break your build (I assume). Given that (1) it's unlikely that people will use the profiler in debug builds, (2) failure of these assertions is likely to lead to deadlocking or crashing in the profiler, and (3) they don't occur on high-frequency paths, I propose that a better fix is to convert them to MOZ_RELEASE_ASSERTs. We should do that in a separate bug. ::: tools/profiler/core/PlatformMacros.h:71 (Diff revision 1) > #elif defined(__linux__) && defined(__arm__) > # define GP_PLAT_arm_linux 1 > # define GP_ARCH_arm 1 > # define GP_OS_linux 1 > > +#elif defined(__linux__) && defined(__aarch64__) I would prefer to use GP_PLAT_arm64_{linux,android} and GP_ARCH_arm64 instead of the _aarch64_ naming, so as to be more consistent with the existing 32-bit arm naming. Please rename these 3 preprocessor symbols accordingly. ::: tools/profiler/lul/LulDwarfSummariser.cpp:313 (Diff revision 1) > + if (mCurrRules.mSPexpr.mHow == UNKNOWN) { > + mCurrRules.mSPexpr = LExpr(NODEREF, DW_REG_CFA, 0); > + } > #elif defined(GP_ARCH_amd64) || defined(GP_ARCH_x86) > > // ---------------- x64/x86 ---------------- // Nit: please put an equivalent spacer comment like this at the start of the arm64 summarisation cases.

(In reply to Julian Seward [:jseward] from comment #3) > Comment on attachment 8967642 [details] > Bug 1450185 - Implement DWARF stack walker for aarch64. > > https://reviewboard.mozilla.org/r/236372/#review244544 > > Thank you for doing this! Basically it looks good. > r+ if you fix the 3 comments below. > > ::: commit-message-da809:9 > (Diff revision 1) > > +not first frame. > > + > > +I test by gtest on Linux/aarch64, and profiler works on Android/aarch64. > > + > > +EM_AARCH64 might not be defined on our builders since headers are old, so > > +this define is needed. And mAdminThreadId is unused on release build and > > Regarding mAdminThreadId, I see what the problem is (yes, it's unused on > release builds). But I don't feel happy about the "Unused << > mAdminThreadId" fix. > > Please remove that change. Clearly this still needs to be fixed, otherwise > the warnings will break your build (I assume). Given that (1) it's unlikely > that people will use the profiler in debug builds, (2) failure of these > assertions is likely to lead to deadlocking or crashing in the profiler, and > (3) they don't occur on high-frequency paths, I propose that a better fix is > to convert them to MOZ_RELEASE_ASSERTs. We should do that in a separate bug. OK, I will remove Unused << mAdminThreadId. But You know, this fix is for the following warning. So, it causes build error on android/aarch64 due to warnings-as-error on task cluster. 4:30.85 warning: tools/profiler/lul/LulMain.h:374:7 [-Wunused-private-field] private field 'mAdminThreadId' is not used Lul constructor is only used by https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/tools/profiler/core/platform-linux-android.cpp#410 and gtests, so it will use on android/aarch64 only. To resolve this unused warning, I will convert it to MOZ_RELEASE_ASSERT to land this if I cannot use Unused << foo.

> (3) they don't occur on high-frequency paths, I propose that a better fix is > to convert them to MOZ_RELEASE_ASSERTs. We should do that in a separate bug. I filed as bug 1456382

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/mozilla-inbound/rev/1893ddb56c3b Implement DWARF stack walker for aarch64. r=jseward