Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2829433003:
Enable frame pointers explicitly under ARM32. (Closed)
DescriptionEnable frame pointers explicitly under ARM32.
This replaces the quick-fix in crrev.com/2820803003, to address issues
with ARM32 builds when frame pointers are disabled.
This CL explicitly enables frame pointers in ARM32 builds, and pulls out
the ARM32 and ARM64 special-cases to be handling separately from Debug,
profiling and sanitizer build configurations, for clarity.
BUG=710131, 706654, 711784
Review-Url: https://codereview.chromium.org/2829433003
Cr-Commit-Position: refs/heads/master@{#466080}
Committed: https://chromium.googlesource.com/chromium/src/+/38f02400540606a7c418f9fa5148d3a07fd8b4fa
Patch Set 1 #
Total comments: 5
Patch Set 2 : Make enable_frame_pointers an argument #Patch Set 3 : Restrict 711784 work-around to ChromeOS #
Messages
Total messages: 36 (18 generated)
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
wez@chromium.org changed reviewers: + dianders@chromium.org, erikchen@chromium.org, gmx@chromium.org
erikchen: PTAL - I think this is in the spirit of your original patch? dianders: Any concerns with this change? :P gmx: Note that I'm enabling frame pointers for all ARM32, not just Thumb, as per your comments on the bug - sounds like we just never hit issues because we happen to always build ARM32+Thumb?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
erikchen@chromium.org changed reviewers: + dskiba@chromium.org
+dskiba https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... build/config/compiler/compiler.gni:97: # We cannot currently unwind ARM32 frame pointers correctly. Technically, we can never unwind all stacks on any platform. See discussion in first 10 comments on Issue 706654. The point of this variable is whether we can unwind "most" stacks. This isn't very satisfactory...but is what we've been doing for heap profiling. I think this will break heap profiling on Chrome on Android. Can we add back the && arm_use_thumb condition? I don't think this should affect ChromeOS, only heap profiling code.
https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... build/config/compiler/compiler.gni:97: # We cannot currently unwind ARM32 frame pointers correctly. On 2017/04/19 02:43:57, erikchen wrote: > Technically, we can never unwind all stacks on any platform. See discussion in > first 10 comments on Issue 706654. The point of this variable is whether we can > unwind "most" stacks. This isn't very satisfactory...but is what we've been > doing for heap profiling. > > I think this will break heap profiling on Chrome on Android. Can we add back the > && arm_use_thumb condition? I don't think this should affect ChromeOS, only heap > profiling code. Yes, this will break Android heap profiler. We can unwind on ARM32, provided that we're not in Thumb mode (this requires manual arm_use_thumb=false). Please change the condition back. As for uwnding "most" stacks - this depends on how malloc is hooked. On Android we're wrapping malloc, i.e. intercepting malloc calls only made by the Chrome itself. In this setup we can unwind all the stacks we need to unwind.
On 2017/04/19 01:51:39, Wez wrote: > erikchen: PTAL - I think this is in the spirit of your original patch? > dianders: Any concerns with this change? :P > gmx: Note that I'm enabling frame pointers for all ARM32, not just Thumb, as per > your comments on the bug - sounds like we just never hit issues because we > happen to always build ARM32+Thumb? I'm pretty oblivious to how Chrome gets built, but as far as I know from all the discussion: * We (temporarily) want frame pointers on Chrome/thumb to work around the A17 bug * We (normally) don't need frame pointers on Chrome/thumb because we can't use them anyway. * For other Chrome architectures, I thought people wanted frame pointers to make CWP work.
https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... build/config/compiler/compiler.gni:84: # triggers some ARM A12/A17 errata. Do we think this really happens on Android versions of Chrome, too? The old condition allowed "no frame pointers" on arm on android.
https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... File build/config/compiler/compiler.gni (right): https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... build/config/compiler/compiler.gni:84: # triggers some ARM A12/A17 errata. On 2017/04/19 17:35:25, diandersAtChromium wrote: > Do we think this really happens on Android versions of Chrome, too? The old > condition allowed "no frame pointers" on arm on android. That's a good question; I don't see any reason why Android vs CrOS would make a difference for a given CPU architecture - if you are confident that this is specific to the combination of Thumb+A12/A17+FPU/Neon then we can make the condition more specific. However, as per our conversation with gmx@, it sounds like even in the absence of this issue we want frame pointers on ChromeOS to support CWP, so I think this condition should become just os_chrome, in fact - we should be explicitly enabling frame pointers for *all* ChromeOS builds, not just ARM32 - following Erik's patch we've actually broken CWP on CrOS x86 & x64, IIUC. https://codereview.chromium.org/2829433003/diff/1/build/config/compiler/compi... build/config/compiler/compiler.gni:97: # We cannot currently unwind ARM32 frame pointers correctly. On 2017/04/19 15:06:09, DmitrySkiba wrote: > On 2017/04/19 02:43:57, erikchen wrote: > > Technically, we can never unwind all stacks on any platform. See discussion in > > first 10 comments on Issue 706654. The point of this variable is whether we > can > > unwind "most" stacks. This isn't very satisfactory...but is what we've been > > doing for heap profiling. > > > > I think this will break heap profiling on Chrome on Android. Can we add back > the > > && arm_use_thumb condition? I don't think this should affect ChromeOS, only > heap > > profiling code. > > Yes, this will break Android heap profiler. We can unwind on ARM32, provided > that we're not in Thumb mode (this requires manual arm_use_thumb=false). Please > change the condition back. Oh, interesting: gmx@ stated that we can't unwind ARM32 stacks properly in general, hence the change. I'll change it back. > As for uwnding "most" stacks - this depends on how malloc is hooked. On Android > we're wrapping malloc, i.e. intercepting malloc calls only made by the Chrome > itself. In this setup we can unwind all the stacks we need to unwind. Right; the reason we're special-casing ARM/Thumb is that even in the ideal case (i.e. nothing in-between frames causing confusion) we can't unwind properly.
On 2017/04/19 18:14:58, Wez wrote: > However, as per our conversation with gmx@, it sounds like even in the absence > of this issue we want frame pointers on ChromeOS to support CWP, so I think this > condition should become just os_chrome, in fact - we should be explicitly > enabling frame pointers for *all* ChromeOS builds, not just ARM32 - following > Erik's patch we've actually broken CWP on CrOS x86 & x64, IIUC. As discussed offline with wez@, the better solution may be for the chrome build system to provide a flag that an external toolchain can set to enable frame pointers, regardless of chrome build type of other chrome heuristics. > > > > Yes, this will break Android heap profiler. We can unwind on ARM32, provided > > that we're not in Thumb mode (this requires manual arm_use_thumb=false). > Please > > change the condition back. > > Oh, interesting: gmx@ stated that we can't unwind ARM32 stacks properly in > general, hence the change. I'll change it back. Our use case is a bit different. Stacks are sampled on many devices, and then aggregated offline to get callgraphs for functions. Unwinds are started asynchronously, from any IP and they cross load modules and user code / kernel code. We found the stacks from ARM32 unreliable and more pain than it was worth it. Because ChromeOS runs also on x86-64, we use only x86-64 callstacks to generate the callgraphs. We still collect flat performance data from ARM32, so we know where time is spent.
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
erikchen, dskiba: PTAL Chatting to gmx@ offline, it sounds like we *don't* need frame pointers for ChromeOS for CWP, except in x86-64, so I think we're back to aiming to be able to switch frame pointers off in ChromeOS ARM builds once the CPU issue is resolved. This version of the cleanup force-enables frame pointers in ARM Thumb builds, and makes enable_frame_pointers a build argument so that ChromeOS can set it to be consistent with the rest of their build in future. The side effect of that is that it's now possible for the configuration to specify enable_frame_pointers=true when in practice it can't work, so I think we'd also want to add some sanity-checking assert()s against bad combinations if we land this.
lgtm thanks!
As I understand, the situation is as follows: * Once frame pointers were explicitly disabled, strange ChromeOS bug emerged, that was found to be ARM32 errata-related * This patch enables frame pointers everywhere on ARM32, because there is a possibility that we might be hitting the same errata on Android devices But we also know that enabling frame pointers regresses performance, and I fear that this CL will be promptly reverted on that ground. Unless of course there is a evidence that we're hitting that errata on Android devices - have we checked crash reports for related crashes? Maybe just leave things as is (i.e. enable frame pointers only for ChromeOS) until we know for sure we have that problem on Android?
No, this only force-enables frame pointers in ARM/Thumb builds; I don't _think_ we build Chrome for Android with ARM/Thumb but I can add an explicit ChromeOS check to avoid the risk. On 20 April 2017 at 09:29, <dskiba@chromium.org> wrote: > As I understand, the situation is as follows: > > * Once frame pointers were explicitly disabled, strange ChromeOS bug > emerged, > that was found to be ARM32 errata-related > > * This patch enables frame pointers everywhere on ARM32, because there is a > possibility that we might be hitting the same errata on Android devices > > But we also know that enabling frame pointers regresses performance, and I > fear > that this CL will be promptly reverted on that ground. > > Unless of course there is a evidence that we're hitting that errata on > Android > devices - have we checked crash reports for related crashes? > > Maybe just leave things as is (i.e. enable frame pointers only for > ChromeOS) > until we know for sure we have that problem on Android? > > https://codereview.chromium.org/2829433003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/20 16:44:03, Wez wrote: > No, this only force-enables frame pointers in ARM/Thumb builds; I don't > _think_ we build Chrome for Android with ARM/Thumb but I can add an > explicit ChromeOS check to avoid the risk. I think we do, see build/config/arm.gni where by default arm_version is set to 7 and arm_use_thumb is set to true. I.e. unless you explicitly change arm_version or arm_use_thumb, we'll build in thumb mode. > > On 20 April 2017 at 09:29, <mailto:dskiba@chromium.org> wrote: > > > As I understand, the situation is as follows: > > > > * Once frame pointers were explicitly disabled, strange ChromeOS bug > > emerged, > > that was found to be ARM32 errata-related > > > > * This patch enables frame pointers everywhere on ARM32, because there is a > > possibility that we might be hitting the same errata on Android devices > > > > But we also know that enabling frame pointers regresses performance, and I > > fear > > that this CL will be promptly reverted on that ground. > > > > Unless of course there is a evidence that we're hitting that errata on > > Android > > devices - have we checked crash reports for related crashes? > > > > Maybe just leave things as is (i.e. enable frame pointers only for > > ChromeOS) > > until we know for sure we have that problem on Android? > > > > https://codereview.chromium.org/2829433003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/20 16:59:04, DmitrySkiba wrote: > On 2017/04/20 16:44:03, Wez wrote: > > No, this only force-enables frame pointers in ARM/Thumb builds; I don't > > _think_ we build Chrome for Android with ARM/Thumb but I can add an > > explicit ChromeOS check to avoid the risk. > > I think we do, see build/config/arm.gni where by default arm_version is set to 7 > and arm_use_thumb is set to true. I.e. unless you explicitly change arm_version > or arm_use_thumb, we'll build in thumb mode. Yeah, I just confirmed that - added a restriction to the TODO() to apply only to ChromeOS in this patch. dianders: Please confirm that this looks reasonable to you, given the problem, or whether it's worth restricting this to specific ARM revisions.
wez@chromium.org changed reviewers: + dpranke@chromium.org
+dpranke for OWNERS
lgtm
I'm not terribly familiar with gn build files, but the concepts here seem right to me now. We could try to restrict it more, but (hopefully) this is temporary for Chrome OS. ...and also it's the lowest risk since it matches what we used to do before. ...so we might as well remove frame pointers on all arm/thumb devices at the same time when we're ready.
The CQ bit was unchecked by wez@chromium.org
The CQ bit was checked by wez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/2829433003/#ps40001 (title: "Restrict 711784 work-around to ChromeOS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492713429953350,
"parent_rev": "ad4c20785bdd5472bf4a79fa1422285b93cb8280", "commit_rev":
"38f02400540606a7c418f9fa5148d3a07fd8b4fa"}
Message was sent while issue was closed.
Description was changed from ========== Enable frame pointers explicitly under ARM32. This replaces the quick-fix in crrev.com/2820803003, to address issues with ARM32 builds when frame pointers are disabled. This CL explicitly enables frame pointers in ARM32 builds, and pulls out the ARM32 and ARM64 special-cases to be handling separately from Debug, profiling and sanitizer build configurations, for clarity. BUG=710131, 706654, 711784 ========== to ========== Enable frame pointers explicitly under ARM32. This replaces the quick-fix in crrev.com/2820803003, to address issues with ARM32 builds when frame pointers are disabled. This CL explicitly enables frame pointers in ARM32 builds, and pulls out the ARM32 and ARM64 special-cases to be handling separately from Debug, profiling and sanitizer build configurations, for clarity. BUG=710131, 706654, 711784 Review-Url: https://codereview.chromium.org/2829433003 Cr-Commit-Position: refs/heads/master@{#466080} Committed: https://chromium.googlesource.com/chromium/src/+/38f02400540606a7c418f9fa5148... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/38f02400540606a7c418f9fa5148... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
