|
|
DescriptionFix stack walking to notice if the frame is obviously not valid.
If the PC is too small, the frame can't be valid.
BUG=677302
Review-Url: https://codereview.chromium.org/2692123005
Cr-Commit-Position: refs/heads/master@{#451355}
Committed: https://chromium.googlesource.com/chromium/src/+/f51b7d1a5db6ae289736b84622494086c54cd49a
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments from wez. #Messages
Total messages: 43 (25 generated)
Description was changed from ========== Fix stack walking to notice if the frame is obviously not valid. If the PC is too small, the frame can't be valid. BUG= ========== to ========== Fix stack walking to notice if the frame is obviously not valid. If the PC is too small, the frame can't be valid. BUG=677302 ==========
The CQ bit was checked by erikchen@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...
erikchen@chromium.org changed reviewers: + mark@chromium.org
mark: Please review. I wrote a test to test the obvious thing [all frames > 32768], but it was failing to distinguish between the correct/wrong cases, since the thread it was running on didn't have this same end-of-stack issue.
LGTM https://codereview.chromium.org/2692123005/diff/1/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2692123005/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:115: if (GetStackFramePC(fp) < 32768) You could go even bigger. On x86_64, you’ll generally have a tremendous pagezero. macOS’ default __PAGEZERO is 1 << 32, for example. But I guess that most things that wind up looking like they’re running from code down here are going to be at smallish addresses, so the existing 32k is fine.
https://codereview.chromium.org/2692123005/diff/1/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2692123005/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:115: if (GetStackFramePC(fp) < 32768) On 2017/02/15 21:05:57, Mark Mentovai wrote: > You could go even bigger. On x86_64, you’ll generally have a tremendous > pagezero. macOS’ default __PAGEZERO is 1 << 32, for example. But I guess that > most things that wind up looking like they’re running from code down here are > going to be at smallish addresses, so the existing 32k is fine. Also, this is run on all plaforms.
The CQ bit was unchecked by erikchen@chromium.org
The CQ bit was checked by erikchen@chromium.org
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: + wez@chromium.org
https://codereview.chromium.org/2692123005/diff/1/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2692123005/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:115: if (GetStackFramePC(fp) < 32768) On 2017/02/15 21:52:45, erikchen wrote: > On 2017/02/15 21:05:57, Mark Mentovai wrote: > > You could go even bigger. On x86_64, you’ll generally have a tremendous > > pagezero. macOS’ default __PAGEZERO is 1 << 32, for example. But I guess that > > most things that wind up looking like they’re running from code down here are > > going to be at smallish addresses, so the existing 32k is fine. > > Also, this is run on all plaforms. Maybe use a kConstant here, so we can define it to a suitable value on a per-platform basis easily in future?
The CQ bit was unchecked by erikchen@chromium.org
https://codereview.chromium.org/2692123005/diff/1/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2692123005/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:115: if (GetStackFramePC(fp) < 32768) On 2017/02/15 22:38:22, Wez wrote: > On 2017/02/15 21:52:45, erikchen wrote: > > On 2017/02/15 21:05:57, Mark Mentovai wrote: > > > You could go even bigger. On x86_64, you’ll generally have a tremendous > > > pagezero. macOS’ default __PAGEZERO is 1 << 32, for example. But I guess > that > > > most things that wind up looking like they’re running from code down here > are > > > going to be at smallish addresses, so the existing 32k is fine. > > > > Also, this is run on all plaforms. > > Maybe use a kConstant here, so we can define it to a suitable value on a > per-platform basis easily in future? Done.
The CQ bit was checked by erikchen@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/2692123005/#ps20001 (title: "Comments from wez.")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by erikchen@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by erikchen@chromium.org
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by erikchen@chromium.org
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by erikchen@chromium.org
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": 20001, "attempt_start_ts": 1487358302238450, "parent_rev": "c42a905d5344ea0424df914329cbc5d35fd0ede3", "commit_rev": "f51b7d1a5db6ae289736b84622494086c54cd49a"}
Message was sent while issue was closed.
Description was changed from ========== Fix stack walking to notice if the frame is obviously not valid. If the PC is too small, the frame can't be valid. BUG=677302 ========== to ========== Fix stack walking to notice if the frame is obviously not valid. If the PC is too small, the frame can't be valid. BUG=677302 Review-Url: https://codereview.chromium.org/2692123005 Cr-Commit-Position: refs/heads/master@{#451355} Committed: https://chromium.googlesource.com/chromium/src/+/f51b7d1a5db6ae289736b8462249... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f51b7d1a5db6ae289736b8462249...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2704933004/ by vabr@chromium.org. The reason for reverting is: This likely caused a consistent breakage on CrOS ASAN/LSAN bot: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... More info on https://crbug.com/677302#c16.
Message was sent while issue was closed.
Description was changed from ========== Fix stack walking to notice if the frame is obviously not valid. If the PC is too small, the frame can't be valid. BUG=677302 Review-Url: https://codereview.chromium.org/2692123005 Cr-Commit-Position: refs/heads/master@{#451355} Committed: https://chromium.googlesource.com/chromium/src/+/f51b7d1a5db6ae289736b8462249... ========== to ========== Fix stack walking to notice if the frame is obviously not valid. If the PC is too small, the frame can't be valid. BUG=677302 Review-Url: https://codereview.chromium.org/2692123005 Cr-Commit-Position: refs/heads/master@{#451355} Committed: https://chromium.googlesource.com/chromium/src/+/f51b7d1a5db6ae289736b8462249... ==========
The CQ bit was checked by erikchen@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/2692123005/#ps40001 (title: "change ordering of two conditionals in IsStackFrameValid().")
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 erikchen@chromium.org
Message was sent while issue was closed.
Patchset #3 (id:40001) has been deleted |