Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848683006:
Implement the NativeStackSampler for the Mac. (Closed)
|
Created:
3 years, 7 months ago by Avi (use Gerrit) Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, mac-reviews_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the NativeStackSampler for the Mac.
This relands the sampler, but with a very simplified recovery with no fancy stack scanning in case libunwind stops after one frame.
BUG=531673
Review-Url: https://codereview.chromium.org/2848683006
Cr-Commit-Position: refs/heads/master@{#469429}
Committed: https://chromium.googlesource.com/chromium/src/+/7533f415b244d51ff62f3216c524ad774ca5549e
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix #Patch Set 3 : min to fix test #
Total comments: 2
Messages
Total messages: 32 (19 generated)
The CQ bit was checked by avi@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...
avi@chromium.org changed reviewers: + mark@chromium.org, thakis@chromium.org, wittman@chromium.org
wittman@chromium.org: Please review changes in stack sampler mark@chromium.org: Please review changes in mac stuff thakis@chromium.org: Please review changes in config file This is identical to https://codereview.chromium.org/2702463003/ which was reverted, except we're not doing the secondary stack scan. If we're lucky, this won't crash, and we can tweak the recovery of single-frame walks to be more careful. If we're unlucky, this will crash and we'll have to re-think the use of libunwind.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
lgtm, with compile fixes https://codereview.chromium.org/2848683006/diff/1/base/profiler/native_stack_... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2848683006/diff/1/base/profiler/native_stack_... base/profiler/native_stack_sampler_mac.cc:136: bool WalkStackFromContext(unw_context_t* unwind_context, nit: remove the unused bool return value
The CQ bit was checked by avi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2848683006/diff/1/base/profiler/native_stack_... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2848683006/diff/1/base/profiler/native_stack_... base/profiler/native_stack_sampler_mac.cc:136: bool WalkStackFromContext(unw_context_t* unwind_context, On 2017/04/28 22:33:10, Mike Wittman wrote: > nit: remove the unused bool return value Done.
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks like we still need enough of the previous code to be able to backtrack from libsystem_kernel.dylib, to make the tests pass.
If it’s identical to what I already reviewed minus what you say you removed, then LGTM on the theory that I knew what I was doing then.
The CQ bit was checked by avi@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.
Mike, ptal. Test works now.
lgtm
Nico, ping.
rs-lgtm
Description was changed from ========== Implement the NativeStackSampler for the Mac. BUG=531673 ========== to ========== Implement the NativeStackSampler for the Mac. This relands the sampler, but with a very simplified recovery with no fancy stack scanning in case libunwind stops after one frame. BUG=531673 ==========
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2848683006/#ps40001 (title: "min to fix test")
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": 1493926633123700,
"parent_rev": "f32f0b0adf54be179edcb3ea613a85ee90cd078a", "commit_rev":
"7533f415b244d51ff62f3216c524ad774ca5549e"}
Message was sent while issue was closed.
Description was changed from ========== Implement the NativeStackSampler for the Mac. This relands the sampler, but with a very simplified recovery with no fancy stack scanning in case libunwind stops after one frame. BUG=531673 ========== to ========== Implement the NativeStackSampler for the Mac. This relands the sampler, but with a very simplified recovery with no fancy stack scanning in case libunwind stops after one frame. BUG=531673 Review-Url: https://codereview.chromium.org/2848683006 Cr-Commit-Position: refs/heads/master@{#469429} Committed: https://chromium.googlesource.com/chromium/src/+/7533f415b244d51ff62f3216c524... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7533f415b244d51ff62f3216c524...
Message was sent while issue was closed.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2848683006/diff/40001/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2848683006/diff/40001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:167: strncpy(path, info.dli_fname, PATH_MAX); Drive-by nit: prefer strlcpy over strncpy as strncpy() doesn't guarantee a null terminator char when hitting the size limit and is less efficient (zeroes the full buffer rather until the null terminator).
Message was sent while issue was closed.
https://codereview.chromium.org/2848683006/diff/40001/base/profiler/native_st... File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2848683006/diff/40001/base/profiler/native_st... base/profiler/native_stack_sampler_mac.cc:167: strncpy(path, info.dli_fname, PATH_MAX); On 2017/05/04 21:01:30, Alexei Svitkine (slow) wrote: > Drive-by nit: prefer strlcpy over strncpy as strncpy() doesn't guarantee a null > terminator char when hitting the size limit and is less efficient (zeroes the > full buffer rather until the null terminator). Addressing in https://codereview.chromium.org/2882453003 . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
