Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1663113002:
[debugger] Use code offsets from frame summary in FromFrame Function. (Closed)
|
Created:
4 years, 10 months ago by zhengxing.li Modified:
4 years ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[debugger] Use code offsets from frame summary in FromFrame Function.
The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible.
But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect.
So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function.
This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion.
Committed: https://crrev.com/a0f6d5ed949da17fe94be22021d14b9006238edf
Cr-Commit-Position: refs/heads/master@{#33778}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Messages
Total messages: 19 (7 generated)
PTAL, thanks!
On 2016/02/03 15:32:16, zhengxing.li wrote: > PTAL, thanks! Thanks for finding this! I dont think the fix is correct though, since we will simply give up with a wild guess. We should use the frame summary to find the corresponding code offset in unoptimized code though.
On 2016/02/04 16:25:40, Yang wrote: > On 2016/02/03 15:32:16, zhengxing.li wrote: > > PTAL, thanks! > > Thanks for finding this! > > I dont think the fix is correct though, since we will simply give up with a wild > guess. We should use the frame summary to find the corresponding code offset in > unoptimized code though. This issue a block issue for keeping our x87 v8 master green. So I submit this CL, but It's more like a remainder for you. Hope you can find a more suitable solution for this issue, thanks!
Description was changed from ========== [debugger] Add code offsets of optimizeed frame check in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL add code offsets of optimizeed frame check in FromFrame Function, only offset from un-optimized frame can get the closest breakpoint index and the offset from optimized frame get the first or the last breakpoint index like what previous V8 does. BUG= ========== to ========== [debugger] Add code offsets of optimizeed frame check in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. ==========
On 2016/02/05 01:08:30, zhengxing.li wrote: > On 2016/02/04 16:25:40, Yang wrote: > > On 2016/02/03 15:32:16, zhengxing.li wrote: > > > PTAL, thanks! > > > > Thanks for finding this! > > > > I dont think the fix is correct though, since we will simply give up with a > wild > > guess. We should use the frame summary to find the corresponding code offset > in > > unoptimized code though. > > This issue a block issue for keeping our x87 v8 master green. So I submit this > CL, but It's more like a remainder for you. > > Hope you can find a more suitable solution for this issue, thanks! Hi, Yang: I updated this CL according to your suggestion, please help me review it. Thanks!
See if I understand this correctly: The bug is not new. We used to try to find the source position from the unoptimized code with a raw address into optimized code because we are in an optimized frame. This did not break because Code::SourcePosition would simply return -1. Now that we use an offset, we confuse the offset into unoptimized code with offset into optimized code, which causes wrong calculation. I think this fix is fine, though there are some issues remaining. https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... src/debug/debug.cc:153: FrameSummary GetFirstFrameSummary(JavaScriptFrame* frame); Can you simply move the function definition to here? https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... src/debug/debug.cc:160: Handle<JSFunction> function(summary.function()); You don't need to refresh the frame summary here. We only do that at other call sites because we may not have the debug info, so we may have to recompile the code. Here we already have the debug info.
On 2016/02/05 08:49:34, Yang wrote: > See if I understand this correctly: > The bug is not new. We used to try to find the source position from the > unoptimized code with a raw address into optimized code because we are in an > optimized frame. This did not break because Code::SourcePosition would simply > return -1. > Now that we use an offset, we confuse the offset into unoptimized code with > offset into optimized code, which causes wrong calculation. > I think this fix is fine, though there are some issues remaining. > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc > File src/debug/debug.cc (right): > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... > src/debug/debug.cc:153: FrameSummary GetFirstFrameSummary(JavaScriptFrame* > frame); > Can you simply move the function definition to here? > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... > src/debug/debug.cc:160: Handle<JSFunction> function(summary.function()); > You don't need to refresh the frame summary here. We only do that at other call > sites because we may not have the debug info, so we may have to recompile the > code. Here we already have the debug info. Yes, It seems that the bug is not new. V8 used to get a raw PC address from an optimized frame. The result is: The origianl BreakIndexFromAddress() just compare the raw PC address with all breakpoint's PC address and return the closest breakpoint's index. For Code::SourcePosition, As far as my understanding, FrameInspector::GetSourcePosition() call Code::SourcePosition() if the frame is un-optimized otherwise it will return the Source position in deoptimized frame. So It's no problem for the CL 33579. I updated the CL according to suggestions in your comments, please review it again. Thanks!
On 2016/02/05 10:35:55, zhengxing.li wrote: > On 2016/02/05 08:49:34, Yang wrote: > > See if I understand this correctly: > > The bug is not new. We used to try to find the source position from the > > unoptimized code with a raw address into optimized code because we are in an > > optimized frame. This did not break because Code::SourcePosition would simply > > return -1. > > Now that we use an offset, we confuse the offset into unoptimized code with > > offset into optimized code, which causes wrong calculation. > > I think this fix is fine, though there are some issues remaining. > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc > > File src/debug/debug.cc (right): > > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... > > src/debug/debug.cc:153: FrameSummary GetFirstFrameSummary(JavaScriptFrame* > > frame); > > Can you simply move the function definition to here? > > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... > > src/debug/debug.cc:160: Handle<JSFunction> function(summary.function()); > > You don't need to refresh the frame summary here. We only do that at other > call > > sites because we may not have the debug info, so we may have to recompile the > > code. Here we already have the debug info. > > Yes, It seems that the bug is not new. V8 used to get a raw PC address from an > optimized frame. > > The result is: > The origianl BreakIndexFromAddress() just compare the raw PC address with all > breakpoint's PC address > and return the closest breakpoint's index. > > For Code::SourcePosition, As far as my understanding, > FrameInspector::GetSourcePosition() call Code::SourcePosition() > if the frame is un-optimized otherwise it will return the Source position in > deoptimized frame. So It's no problem for the > CL 33579. > > I updated the CL according to suggestions in your comments, please review it > again. Thanks! BTW, Who has the LGTM right for src/debug? you, rmcilroy or vogelheim? Thanks!
Description was changed from ========== [debugger] Add code offsets of optimizeed frame check in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. ========== to ========== [debugger] Use code offsets from frame summary in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. ==========
Description was changed from ========== [debugger] Use code offsets from frame summary in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. ========== to ========== [debugger] Use code offsets from frame summary in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. ==========
On 2016/02/05 10:38:10, zhengxing.li wrote: > On 2016/02/05 10:35:55, zhengxing.li wrote: > > On 2016/02/05 08:49:34, Yang wrote: > > > See if I understand this correctly: > > > The bug is not new. We used to try to find the source position from the > > > unoptimized code with a raw address into optimized code because we are in an > > > optimized frame. This did not break because Code::SourcePosition would > simply > > > return -1. > > > Now that we use an offset, we confuse the offset into unoptimized code with > > > offset into optimized code, which causes wrong calculation. > > > I think this fix is fine, though there are some issues remaining. > > > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc > > > File src/debug/debug.cc (right): > > > > > > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... > > > src/debug/debug.cc:153: FrameSummary GetFirstFrameSummary(JavaScriptFrame* > > > frame); > > > Can you simply move the function definition to here? > > > > > > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... > > > src/debug/debug.cc:160: Handle<JSFunction> function(summary.function()); > > > You don't need to refresh the frame summary here. We only do that at other > > call > > > sites because we may not have the debug info, so we may have to recompile > the > > > code. Here we already have the debug info. > > > > Yes, It seems that the bug is not new. V8 used to get a raw PC address from an > > optimized frame. > > > > The result is: > > The origianl BreakIndexFromAddress() just compare the raw PC address with all > > breakpoint's PC address > > and return the closest breakpoint's index. > > > > For Code::SourcePosition, As far as my understanding, > > FrameInspector::GetSourcePosition() call Code::SourcePosition() > > if the frame is un-optimized otherwise it will return the Source position in > > deoptimized frame. So It's no problem for the > > CL 33579. > > > > I updated the CL according to suggestions in your comments, please review it > > again. Thanks! > > BTW, Who has the LGTM right for src/debug? you, rmcilroy or vogelheim? > > Thanks! LGTM. I'm owner of src/debug. So you can click on the "commit" checkbox to commit this change.
The CQ bit was checked by zhengxing.li@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1663113002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1663113002/40001
Message was sent while issue was closed.
Description was changed from ========== [debugger] Use code offsets from frame summary in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. ========== to ========== [debugger] Use code offsets from frame summary in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [debugger] Use code offsets from frame summary in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. ========== to ========== [debugger] Use code offsets from frame summary in FromFrame Function. The CL 33579 (https://codereview.chromium.org/1618343002) use code offsets instead of raw PC where possible. But the offset maybe come from an optimized frame, not the un-optimized frame that FromCodeOffset and BreakIndexFromCodeOffset function expect. So The offset from optimized frame can't be used in FromCodeOffset and BreakIndexFromCodeOffset function. This CL use the frame summary to find the corresponding code offset in unoptimized code according to Yang's suggestion. Committed: https://crrev.com/a0f6d5ed949da17fe94be22021d14b9006238edf Cr-Commit-Position: refs/heads/master@{#33778} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a0f6d5ed949da17fe94be22021d14b9006238edf Cr-Commit-Position: refs/heads/master@{#33778}
Message was sent while issue was closed.
On 2016/02/05 11:49:40, Yang wrote: > On 2016/02/05 10:38:10, zhengxing.li wrote: > > On 2016/02/05 10:35:55, zhengxing.li wrote: > > > On 2016/02/05 08:49:34, Yang wrote: > > > > See if I understand this correctly: > > > > The bug is not new. We used to try to find the source position from the > > > > unoptimized code with a raw address into optimized code because we are in > an > > > > optimized frame. This did not break because Code::SourcePosition would > > simply > > > > return -1. > > > > Now that we use an offset, we confuse the offset into unoptimized code > with > > > > offset into optimized code, which causes wrong calculation. > > > > I think this fix is fine, though there are some issues remaining. > > > > > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc > > > > File src/debug/debug.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... > > > > src/debug/debug.cc:153: FrameSummary GetFirstFrameSummary(JavaScriptFrame* > > > > frame); > > > > Can you simply move the function definition to here? > > > > > > > > > > > > > > https://codereview.chromium.org/1663113002/diff/20001/src/debug/debug.cc#newc... > > > > src/debug/debug.cc:160: Handle<JSFunction> function(summary.function()); > > > > You don't need to refresh the frame summary here. We only do that at other > > > call > > > > sites because we may not have the debug info, so we may have to recompile > > the > > > > code. Here we already have the debug info. > > > > > > Yes, It seems that the bug is not new. V8 used to get a raw PC address from > an > > > optimized frame. > > > > > > The result is: > > > The origianl BreakIndexFromAddress() just compare the raw PC address with > all > > > breakpoint's PC address > > > and return the closest breakpoint's index. > > > > > > For Code::SourcePosition, As far as my understanding, > > > FrameInspector::GetSourcePosition() call Code::SourcePosition() > > > if the frame is un-optimized otherwise it will return the Source position in > > > deoptimized frame. So It's no problem for the > > > CL 33579. > > > > > > I updated the CL according to suggestions in your comments, please review it > > > again. Thanks! > > > > BTW, Who has the LGTM right for src/debug? you, rmcilroy or vogelheim? > > > > Thanks! > > LGTM. I'm owner of src/debug. So you can click on the "commit" checkbox to > commit this change. Done, thanks Yang. |
