Omit frame-pointers for ChromeOS ARM 32-bit builds |
|||
Issue descriptionI'm not sure I 100% followed whether we (ideally) wanted to leave frame pointers on for arm32 Chrome or if we wanted frame pointers off. IIRC: * Frame pointers needed to be on for profiling in Chrome OS * Profiling is broken (currently) when we use Thumb mode. * We use Thumb mode. * If there's no actual reason to use frame pointers, we should turn them off since we'll get a speedup. --- Most of the details about frame pointers were talked about in bug #706654 . The final commit there reads: > Since ChromeOS needs frame-pointers enabled in x64 builds, to support > CWP, and temporarily in ARM/Thumb builds, to avoid CPU errata, we now > always enable them under ChromeOS. The ChromeOS toolchain already set > frame-pointers always on so this restores the pre-M59 behaviour. That makes it sound like it's expected that we _should_ disable frame pointers now. ...but hopefully someone who knows more can confirm. --- Said another way: I believe that the CPU errata has been root caused. If everyone thinks that means we should turn frame pointers off for arm32, then go for it! See bug #711784 for details about the root cause.
,
Jul 12 2017
My understanding is that we do want frame pointers on Intel, for some sort of profiling tooling, but we don't want them on ARM. I will update the Chromium-side compiler.gni to stop enabling them in ARM builds, but can one of you confirm the Intel requirement?
,
Jul 12 2017
Yes, we want frame pointers on Intel. And perhaps even on ARM64 if we ever get 64-bit ARM builds. It's fine to disable on arm32.
,
Jul 12 2017
,
Jul 12 2017
> And perhaps even on ARM64 if we ever get 64-bit ARM builds. I think you want them mostly to deal with a current breakpad bug (not how/when crashpad deals with that), see Issue 601759 On arm32 instead frame pointers are pretty useless unless you build in ARM (i.e. NON-Thumb) mode (but that is 2x larger). See https://github.com/google/sanitizers/issues/640 and https://bugs.llvm.org/show_bug.cgi?id=18505
,
Jul 12 2017
Re: #5, I was speaking from CWP's point of view in response to #2. I wasn't speaking on behalf of everybody, as I am not working on Chrome.
,
Jul 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5cddc222d7e76891b197e853d8d61a1f6843b24f commit 5cddc222d7e76891b197e853d8d61a1f6843b24f Author: Wez <wez@chromium.org> Date: Thu Jul 13 00:00:02 2017 Update the enable_frame_pointers logic for ChromeOS. Now that we have a patch for the ARM A12/A17 errata, it is safe to try building ARM Thumb without frame-pointers again, since we don't need them. We leave frame-pointers enabled in ChromeOS for 64-bit. The Windows case is tweaked to use current_cpu rather than target_cpu, and the Android case is explicitly gated on is_android. Bug: 740806 , 711784 Change-Id: I2fff46d1a3a8d78d12c389f95a20acf54013318d Reviewed-on: https://chromium-review.googlesource.com/567608 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#486165} [modify] https://crrev.com/5cddc222d7e76891b197e853d8d61a1f6843b24f/build/config/compiler/compiler.gni
,
Jul 13 2017
Gah, failed to update my CL description after moving the Windows & Android stuff out. :( |
|||
►
Sign in to add a comment |
|||
Comment 1 by llozano@chromium.org
, Jul 11 2017