Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1763783003:
[Interpreter] Fixes translation from bailout id to code offset. (Closed)
Description[Interpreter] Fixes translation from bailout id to code offset.
BailoutId points to the next bytecode in the bytecode array. Code offset
is set to one less than the bail out id. This would point to the end of the
current instruction. Since we use it only for summarizing the frame and to
compute the source position, it should be safe to set it to the end of current
instruction.
BUG=v8:4280, v8:4689
LOG=N
Committed: https://crrev.com/01f603d2b28d7d7402a1ce468c7a6b1aeb69b9e9
Cr-Commit-Position: refs/heads/master@{#34580}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added comments #
Messages
Total messages: 28 (13 generated)
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763783003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763783003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mythria@chromium.org changed reviewers: + rmcilroy@chromium.org
Ross, This is not a proper fix. I just uploaded this to show you the problem. https://codereview.chromium.org/1763783003/diff/1/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1763783003/diff/1/src/frames.cc#newcode1016 src/frames.cc:1016: code_offset = bailout_id.ToInt() - 1; This is not a proper fix, but works. As Yang, pointed out bailout id is the next instruction and not the current node id. When we subtract 1 we will be at the last bytecode of the current instruction. Source position returns the same value if we are at the beginning or the end of the current instruction. I am afraid this may break somewhere. I don't think I can commit this. I was trying to find a proper fix, but I couldn't come up with any :(.
rmcilroy@chromium.org changed reviewers: + yangguo@chromium.org
https://codereview.chromium.org/1763783003/diff/1/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1763783003/diff/1/src/frames.cc#newcode1016 src/frames.cc:1016: code_offset = bailout_id.ToInt() - 1; On 2016/03/04 16:15:33, mythria wrote: > This is not a proper fix, but works. As Yang, pointed out bailout id is the next > instruction and not the current node id. When we subtract 1 we will be at the > last bytecode of the current instruction. Source position returns the same value > if we are at the beginning or the end of the current instruction. I am afraid > this may break somewhere. > I don't think I can commit this. I was trying to find a proper fix, but I > couldn't come up with any :(. I'm not sure this is a problem, did you find anything which it fails due to? The source position for the current bytecode should always end after the last byte of that bytecode (i.e., on the first byte of the next bytecode), so even although this is not necessarily on a bytecode boundary, it should still always fall in the valid range for the correct source position AFAIK. Yang, what do you think?
On 2016/03/05 03:01:25, rmcilroy (Slow - Traveling) wrote: > https://codereview.chromium.org/1763783003/diff/1/src/frames.cc > File src/frames.cc (right): > > https://codereview.chromium.org/1763783003/diff/1/src/frames.cc#newcode1016 > src/frames.cc:1016: code_offset = bailout_id.ToInt() - 1; > On 2016/03/04 16:15:33, mythria wrote: > > This is not a proper fix, but works. As Yang, pointed out bailout id is the > next > > instruction and not the current node id. When we subtract 1 we will be at the > > last bytecode of the current instruction. Source position returns the same > value > > if we are at the beginning or the end of the current instruction. I am afraid > > this may break somewhere. > > I don't think I can commit this. I was trying to find a proper fix, but I > > couldn't come up with any :(. > > I'm not sure this is a problem, did you find anything which it fails due to? The > source position for the current bytecode should always end after the last byte > of that bytecode (i.e., on the first byte of the next bytecode), so even > although this is not necessarily on a bytecode boundary, it should still always > fall in the valid range for the correct source position AFAIK. Yang, what do you > think? Afaict the code_offset value is only used to find break locations and to calculate source positions. For both purposes I think this should work. However, we should have some way to guard against using code_offset as exact offset to the bytecode start to be future-proof. I'm surprised that ignition/optimized-debug-frame is fixed as well. Does the FrameInspector already correctly translate optimized code PC to bytecode offset?
On 2016/03/07 06:36:33, Yang wrote: > On 2016/03/05 03:01:25, rmcilroy (Slow - Traveling) wrote: > > https://codereview.chromium.org/1763783003/diff/1/src/frames.cc > > File src/frames.cc (right): > > > > https://codereview.chromium.org/1763783003/diff/1/src/frames.cc#newcode1016 > > src/frames.cc:1016: code_offset = bailout_id.ToInt() - 1; > > On 2016/03/04 16:15:33, mythria wrote: > > > This is not a proper fix, but works. As Yang, pointed out bailout id is the > > next > > > instruction and not the current node id. When we subtract 1 we will be at > the > > > last bytecode of the current instruction. Source position returns the same > > value > > > if we are at the beginning or the end of the current instruction. I am > afraid > > > this may break somewhere. > > > I don't think I can commit this. I was trying to find a proper fix, but I > > > couldn't come up with any :(. > > > > I'm not sure this is a problem, did you find anything which it fails due to? > The > > source position for the current bytecode should always end after the last byte > > of that bytecode (i.e., on the first byte of the next bytecode), so even > > although this is not necessarily on a bytecode boundary, it should still > always > > fall in the valid range for the correct source position AFAIK. Yang, what do > you > > think? > > > Afaict the code_offset value is only used to find break locations and to > calculate source positions. For both purposes I think this should work. However, > we should have some way to guard against using code_offset as exact offset to > the bytecode start to be future-proof. > > I'm surprised that ignition/optimized-debug-frame is fixed as well. Does the > FrameInspector already correctly translate optimized code PC to bytecode offset? If we really want it to be exact we could iterate through the bytecode up to that code offset to obtain the offset for the previous bytecode...
Description was changed from ========== [Interpreter] Fixes translation from bailout id to code offset - NOT FOR COMMIT. BUG=v8:4280, v8:4689 LOG=N ========== to ========== [Interpreter] Fixes translation from bailout id to code offset. BailoutId points to the next bytecode in the bytecode array. Code offset is set to one less than the bail out id. This would point to the end of the current instruction. Since we use it only for summarizing the frame and to compute the source position, it should be safe to set it to the end of current instruction. BUG=v8:4280, v8:4689 LOG=N ==========
Description was changed from ========== [Interpreter] Fixes translation from bailout id to code offset. BailoutId points to the next bytecode in the bytecode array. Code offset is set to one less than the bail out id. This would point to the end of the current instruction. Since we use it only for summarizing the frame and to compute the source position, it should be safe to set it to the end of current instruction. BUG=v8:4280, v8:4689 LOG=N ========== to ========== [Interpreter] Fixes translation from bailout id to code offset. BailoutId points to the next bytecode in the bytecode array. Code offset is set to one less than the bail out id. This would point to the end of the current instruction. Since we use it only for summarizing the frame and to compute the source position, it should be safe to set it to the end of current instruction. BUG=v8:4280, v8:4689 LOG=N ==========
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763783003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763783003/20001
PTAL. Yang, I had an offline discussion with Ross, and we think it is ok to use bailout_id - 1, because we use it to only compute the sourceposition or to summarize frames. Let me know if you think its not safe. Thanks, Mythri
On 2016/03/08 11:40:01, mythria wrote: > PTAL. > > Yang, > I had an offline discussion with Ross, and we think it is ok to use bailout_id - > 1, because we use it to only compute the sourceposition or to summarize frames. > Let me know if you think its not safe. > > Thanks, > Mythri I'm fine with this as well. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks.
Thanks for the review.
The CQ bit was checked by mythria@chromium.org
The CQ bit was unchecked by mythria@chromium.org
The CQ bit was checked by mythria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763783003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763783003/20001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Fixes translation from bailout id to code offset. BailoutId points to the next bytecode in the bytecode array. Code offset is set to one less than the bail out id. This would point to the end of the current instruction. Since we use it only for summarizing the frame and to compute the source position, it should be safe to set it to the end of current instruction. BUG=v8:4280, v8:4689 LOG=N ========== to ========== [Interpreter] Fixes translation from bailout id to code offset. BailoutId points to the next bytecode in the bytecode array. Code offset is set to one less than the bail out id. This would point to the end of the current instruction. Since we use it only for summarizing the frame and to compute the source position, it should be safe to set it to the end of current instruction. BUG=v8:4280, v8:4689 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Fixes translation from bailout id to code offset. BailoutId points to the next bytecode in the bytecode array. Code offset is set to one less than the bail out id. This would point to the end of the current instruction. Since we use it only for summarizing the frame and to compute the source position, it should be safe to set it to the end of current instruction. BUG=v8:4280, v8:4689 LOG=N ========== to ========== [Interpreter] Fixes translation from bailout id to code offset. BailoutId points to the next bytecode in the bytecode array. Code offset is set to one less than the bail out id. This would point to the end of the current instruction. Since we use it only for summarizing the frame and to compute the source position, it should be safe to set it to the end of current instruction. BUG=v8:4280, v8:4689 LOG=N Committed: https://crrev.com/01f603d2b28d7d7402a1ce468c7a6b1aeb69b9e9 Cr-Commit-Position: refs/heads/master@{#34580} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/01f603d2b28d7d7402a1ce468c7a6b1aeb69b9e9 Cr-Commit-Position: refs/heads/master@{#34580} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
