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

Issue 1407513003: [Interpreter]: Basic support for iterating interpreter stack frames for GC. (Closed)

Created:
5 years, 2 months ago by rmcilroy
Modified:
5 years, 2 months ago
Reviewers:
titzer, oth
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

[Interpreter]: Basic support for iterating interpreter stack frames for GC. Adds basic support for iterating interpreter stack frames for GC. Currently InterpreterStackFrames are treated just like JavaScriptStackFrames since the JavaScriptFrame::IterateExpressions() will correctly iterate over all the local / temp interpeter Registers, and will iterate over the interpreter_entry_trampoline pc address. There is no need to explicitly iterate over the BytecodeArray object since that is held in a machine register in the bytecode handler which is marked as kMachTaggedAny by TurboFan, and so will get iterated appropriately when iterating the bytecode handler stub's stack frame. BUG=v8:4280 LOG=N Committed: https://crrev.com/4b2fffae4c42f0e95333f1f3466c5d8ba7ee4585 Cr-Commit-Position: refs/heads/master@{#31342}

Patch Set 1 : #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -21 lines) Patch
M src/frames.h View 2 chunks +21 lines, -9 lines 2 comments Download
M src/frames.cc View 2 chunks +13 lines, -6 lines 2 comments Download
M src/frames-inl.h View 1 chunk +4 lines, -0 lines 0 comments Download
M src/objects.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/objects-inl.h View 2 chunks +8 lines, -6 lines 0 comments Download

Messages

Total messages: 27 (7 generated)
rmcilroy
This adds basic GC iteration support - exception printing / stackwalking isn't supported for interpreted ...
5 years, 2 months ago (2015-10-14 10:23:07 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407513003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407513003/40001
5 years, 2 months ago (2015-10-14 11:27:30 UTC) #6
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 2 months ago (2015-10-14 12:04:57 UTC) #8
oth
Looks fine here. Pleasantly surprised at how small this turned out to be. https://codereview.chromium.org/1407513003/diff/40001/src/frames.h File ...
5 years, 2 months ago (2015-10-14 12:19:32 UTC) #10
rmcilroy
https://codereview.chromium.org/1407513003/diff/40001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/1407513003/diff/40001/src/frames.h#newcode107 src/frames.h:107: V(INTERPRETED, InterpretedFrame) \ On 2015/10/14 12:19:32, oth wrote: > ...
5 years, 2 months ago (2015-10-14 14:18:47 UTC) #11
oth
On 2015/10/14 14:18:47, rmcilroy wrote: > https://codereview.chromium.org/1407513003/diff/40001/src/frames.h > File src/frames.h (right): > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.h#newcode107 > ...
5 years, 2 months ago (2015-10-14 14:51:19 UTC) #12
titzer
On 2015/10/14 14:51:19, oth wrote: > On 2015/10/14 14:18:47, rmcilroy wrote: > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.h > ...
5 years, 2 months ago (2015-10-15 17:01:40 UTC) #13
titzer
https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 src/frames.cc:442: case Code::BUILTIN: Are interpreter frames really only identifiable through ...
5 years, 2 months ago (2015-10-15 17:01:56 UTC) #14
rmcilroy
https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 src/frames.cc:442: case Code::BUILTIN: On 2015/10/15 17:01:55, titzer wrote: > Are ...
5 years, 2 months ago (2015-10-15 19:47:47 UTC) #15
titzer
On 2015/10/15 19:47:47, rmcilroy wrote: > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc > File src/frames.cc (right): > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc#newcode442 > ...
5 years, 2 months ago (2015-10-15 19:52:05 UTC) #16
rmcilroy
On 2015/10/15 19:52:05, titzer wrote: > On 2015/10/15 19:47:47, rmcilroy wrote: > > https://codereview.chromium.org/1407513003/diff/40001/src/frames.cc > ...
5 years, 2 months ago (2015-10-15 20:40:06 UTC) #17
titzer
On 2015/10/15 20:40:06, rmcilroy wrote: > On 2015/10/15 19:52:05, titzer wrote: > > On 2015/10/15 ...
5 years, 2 months ago (2015-10-16 02:33:39 UTC) #18
titzer
On 2015/10/15 20:40:06, rmcilroy wrote: > On 2015/10/15 19:52:05, titzer wrote: > > On 2015/10/15 ...
5 years, 2 months ago (2015-10-16 02:33:40 UTC) #19
rmcilroy
On 2015/10/16 02:33:40, titzer wrote: > On 2015/10/15 20:40:06, rmcilroy wrote: > > On 2015/10/15 ...
5 years, 2 months ago (2015-10-16 10:50:02 UTC) #20
titzer
On 2015/10/16 10:50:02, rmcilroy wrote: > On 2015/10/16 02:33:40, titzer wrote: > > On 2015/10/15 ...
5 years, 2 months ago (2015-10-16 13:10:27 UTC) #21
rmcilroy
> I was just concerned about the DCHECK in frames.cc that it's the interpreter > ...
5 years, 2 months ago (2015-10-16 14:00:00 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407513003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407513003/40001
5 years, 2 months ago (2015-10-16 14:00:22 UTC) #24
commit-bot: I haz the power
Committed patchset #1 (id:40001)
5 years, 2 months ago (2015-10-16 15:06:16 UTC) #25
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/4b2fffae4c42f0e95333f1f3466c5d8ba7ee4585 Cr-Commit-Position: refs/heads/master@{#31342}
5 years, 2 months ago (2015-10-16 15:06:36 UTC) #26
titzer
5 years, 2 months ago (2015-10-16 16:57:02 UTC) #27
Message was sent while issue was closed.
On 2015/10/16 14:00:00, rmcilroy wrote:
> > I was just concerned about the DCHECK in frames.cc that it's the interpreter
> > entry point is pretty weak.
> > 
> > I'll lgtm this for now, but I'm wondering if it isn't going to be brittle
and
> > bite us.
> 
> Thanks. I'm hoping the DCHECK isn't brittle - INTERPRETED frames will always
> have a return address of the trampoline, but I'll keep an eye on this and
rework
> it if we end up seeing brittleness.

Ok, sgtm. I'll also be in here mucking things up for WebAssembly frames in not
too long.

Powered by Google App Engine
This is Rietveld 408576698