Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(103)

Issue 1527253002: [debugger] simplify step over recursive function call. (Closed)

Created:
5 years ago by Yang
Modified:
5 years ago
Reviewers:
Jakob Kummerow
CC:
v8-reviews_googlegroups.com, Yang
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[debugger] simplify step over recursive function call. The problem is this: when stepping over a recursive function call, the recursive function is flooded with one-shot break points so that we break after the call, but since the callee is the same function, the callee is also flooded, resulting a break in the callee. That however would have been a "step in" instead of "step over". The original solution was to recognize this by comparing FP. If we end up in Debug::Break, we still have to check the current FP against the remembered FP to see whether we are on the same stack height. If we are deeper, then it's not a "step over", and we do not trigger a debug break event. In that case, we queue up the step-over, and temporarily step out until we hit the desired stack height. Note that in order to step out, we flood the caller, which in our example is the same function as the callee. So we break at every flooded break location, and comparing with FP to make sure we stepped out prevents us from triggering debug break events. The new solution simply ignores breaks when the FP compare fails. We simply carry on until we hit a break where the FP compare succeeds. There is no need to do a step out. The number of calls to Debug::Break that do not trigger a debug break event due to failing FP compare is the same. But the code is a lot easier to read. R=jkummerow@chromium.org Committed: https://crrev.com/2bb6e197eda427c39bb03e160949b20537bf3efa Cr-Commit-Position: refs/heads/master@{#32897}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -39 lines) Patch
M src/debug/debug.h View 1 chunk +0 lines, -3 lines 0 comments Download
M src/debug/debug.cc View 2 chunks +5 lines, -36 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
Yang
5 years ago (2015-12-16 10:17:33 UTC) #1
Jakob Kummerow
lgtm
5 years ago (2015-12-16 13:07:53 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1527253002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1527253002/1
5 years ago (2015-12-16 13:09:52 UTC) #4
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years ago (2015-12-16 13:32:49 UTC) #5
commit-bot: I haz the power
5 years ago (2015-12-16 13:33:50 UTC) #7
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/2bb6e197eda427c39bb03e160949b20537bf3efa
Cr-Commit-Position: refs/heads/master@{#32897}

Powered by Google App Engine
This is Rietveld 408576698