Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067503002:
[debugger] fix stepping over await calls for ignition generators. (Closed)
Description[debugger] fix stepping over await calls for ignition generators.
R=bmeurer@chromium.org, caitpotter88@gmail.com, neis@chromium.org
BUG=v8:5099
Committed: https://crrev.com/098964544aad30ad9bda91639962e9ff951db09b
Cr-Commit-Position: refs/heads/master@{#36964}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : address comments #
Depends on Patchset: Messages
Total messages: 35 (13 generated)
The CQ bit was checked by yangguo@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/2067503002/1
https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter.cc:1801: __ CallRuntime(Runtime::kDebugRecordAsyncFunction, context, generator); Is it possible to know at this point in the interpreter whether or not the generator being suspended is an async function or not? Or does it just not matter since it only applies to debugging anyways?
I actually have a different idea. If I understood correctly there is a 1:1 relationship of JSFunction to JSGeneratorObject. We would then record the JSFunction instead of the JSGeneratorObject. And instead of recording it at suspend, we would record it at Debug::PrepareStep. In the resume trampoline we would simply compare the function. I'll implement that tomorrow. https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter.cc:1801: __ CallRuntime(Runtime::kDebugRecordAsyncFunction, context, generator); On 2016/06/13 13:38:13, caitp wrote: > Is it possible to know at this point in the interpreter whether or not the > generator being suspended is an async function or not? Or does it just not > matter since it only applies to debugging anyways? We could expand a bit more and load the function of the generator, and load the shared function info, and then load the bitfield... But that's pretty cumbersome, and probably doesn't matter. We only do this when debugger is active.
On 2016/06/13 13:42:26, Yang wrote: > I actually have a different idea. > > If I understood correctly there is a 1:1 relationship of JSFunction to > JSGeneratorObject. > > We would then record the JSFunction instead of the JSGeneratorObject. And > instead of recording it at suspend, we would record it at Debug::PrepareStep. > > In the resume trampoline we would simply compare the function. > > I'll implement that tomorrow. > > https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc > File src/interpreter/interpreter.cc (right): > > https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... > src/interpreter/interpreter.cc:1801: __ > CallRuntime(Runtime::kDebugRecordAsyncFunction, context, generator); > On 2016/06/13 13:38:13, caitp wrote: > > Is it possible to know at this point in the interpreter whether or not the > > generator being suspended is an async function or not? Or does it just not > > matter since it only applies to debugging anyways? > > We could expand a bit more and load the function of the generator, and load the > shared function info, and then load the bitfield... > > But that's pretty cumbersome, and probably doesn't matter. We only do this when > debugger is active. hm... the relationship is not actually 1:1, is it? And there is no easy way to get the current generator object from Debug::PrepareStep. Nvm.
On 2016/06/13 13:48:32, Yang wrote: > On 2016/06/13 13:42:26, Yang wrote: > > I actually have a different idea. > > > > If I understood correctly there is a 1:1 relationship of JSFunction to > > JSGeneratorObject. > > > > We would then record the JSFunction instead of the JSGeneratorObject. And > > instead of recording it at suspend, we would record it at Debug::PrepareStep. > > > > In the resume trampoline we would simply compare the function. > > > > I'll implement that tomorrow. > > > > > https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc > > File src/interpreter/interpreter.cc (right): > > > > > https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... > > src/interpreter/interpreter.cc:1801: __ > > CallRuntime(Runtime::kDebugRecordAsyncFunction, context, generator); > > On 2016/06/13 13:38:13, caitp wrote: > > > Is it possible to know at this point in the interpreter whether or not the > > > generator being suspended is an async function or not? Or does it just not > > > matter since it only applies to debugging anyways? > > > > We could expand a bit more and load the function of the generator, and load > the > > shared function info, and then load the bitfield... > > > > But that's pretty cumbersome, and probably doesn't matter. We only do this > when > > debugger is active. > > hm... the relationship is not actually 1:1, is it? And there is no easy way to > get the current generator object from Debug::PrepareStep. Nvm. The alternative to this is to introduce a new bytecode for SuspendGenerator that includes the debug runtime call. When copying the bytecode array for debugging we would patch every SuspendGenerator to that new bytecode. I'm not sure that's worthwhile.
On 2016/06/13 13:48:32, Yang wrote: > On 2016/06/13 13:42:26, Yang wrote: > > I actually have a different idea. > > > > If I understood correctly there is a 1:1 relationship of JSFunction to > > JSGeneratorObject. > > > > We would then record the JSFunction instead of the JSGeneratorObject. And > > instead of recording it at suspend, we would record it at Debug::PrepareStep. > > > > In the resume trampoline we would simply compare the function. > > > > I'll implement that tomorrow. > > > > > https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc > > File src/interpreter/interpreter.cc (right): > > > > > https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... > > src/interpreter/interpreter.cc:1801: __ > > CallRuntime(Runtime::kDebugRecordAsyncFunction, context, generator); > > On 2016/06/13 13:38:13, caitp wrote: > > > Is it possible to know at this point in the interpreter whether or not the > > > generator being suspended is an async function or not? Or does it just not > > > matter since it only applies to debugging anyways? > > > > We could expand a bit more and load the function of the generator, and load > the > > shared function info, and then load the bitfield... > > > > But that's pretty cumbersome, and probably doesn't matter. We only do this > when > > debugger is active. > > hm... the relationship is not actually 1:1, is it? And there is no easy way to > get the current generator object from Debug::PrepareStep. Nvm. Why is it not 1:1?
On 2016/06/13 13:59:12, neis wrote: > On 2016/06/13 13:48:32, Yang wrote: > > On 2016/06/13 13:42:26, Yang wrote: > > > I actually have a different idea. > > > > > > If I understood correctly there is a 1:1 relationship of JSFunction to > > > JSGeneratorObject. > > > > > > We would then record the JSFunction instead of the JSGeneratorObject. And > > > instead of recording it at suspend, we would record it at > Debug::PrepareStep. > > > > > > In the resume trampoline we would simply compare the function. > > > > > > I'll implement that tomorrow. > > > > > > > > > https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc > > > File src/interpreter/interpreter.cc (right): > > > > > > > > > https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... > > > src/interpreter/interpreter.cc:1801: __ > > > CallRuntime(Runtime::kDebugRecordAsyncFunction, context, generator); > > > On 2016/06/13 13:38:13, caitp wrote: > > > > Is it possible to know at this point in the interpreter whether or not the > > > > generator being suspended is an async function or not? Or does it just not > > > > matter since it only applies to debugging anyways? > > > > > > We could expand a bit more and load the function of the generator, and load > > the > > > shared function info, and then load the bitfield... > > > > > > But that's pretty cumbersome, and probably doesn't matter. We only do this > > when > > > debugger is active. > > > > hm... the relationship is not actually 1:1, is it? And there is no easy way to > > get the current generator object from Debug::PrepareStep. Nvm. > > Why is it not 1:1? function* f() {yield 1} var f1 = f() var f2 = f() f1 and f2 are both generator objects, right? or am I misunderstanding something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter-assembler.cc:625: MachineType::Pointer(), This doesn't look quite right here. The is_active_ field is a bool, so we probably need MachineType::Int8() here, although I'm not sure that C++ always stores bool into int8. Maybe we can explicitly change the fields in debug.h to int8_t to be sure?
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
> Ross, can you comment on whether you could live > with the additional code for each yield? I can live with this. The isdebbuggeractive check shouldn't be very expensive in relation to saving the register file. > The alternative to this is to introduce a > new bytecode for SuspendGenerator that > includes the debug runtime call. When > copying the bytecode array for debugging > we would patch every SuspendGenerator to > that new bytecode. I don't think that would be worth it just to save this one branch. https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter-assembler.cc:625: MachineType::Pointer(), On 2016/06/14 03:45:36, Benedikt Meurer wrote: > This doesn't look quite right here. The is_active_ field is a bool, so we > probably need MachineType::Int8() here, although I'm not sure that C++ always > stores bool into int8. Maybe we can explicitly change the fields in debug.h to > int8_t to be sure? Yes, it's implementation defined. It's probably always a byte on compilers we use, but I agree we should change the field to be sure. https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter.cc:1797: Label if_debugging(assembler), debugger_done(assembler); Please make if_debugging deferred. (It's the only call in this handler, so if it's deferred the handler won't build a frame unless it enters this branch).
https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter-assembler.cc:625: MachineType::Pointer(), On 2016/06/14 06:23:11, rmcilroy wrote: > On 2016/06/14 03:45:36, Benedikt Meurer wrote: > > This doesn't look quite right here. The is_active_ field is a bool, so we > > probably need MachineType::Int8() here, although I'm not sure that C++ always > > stores bool into int8. Maybe we can explicitly change the fields in debug.h to > > int8_t to be sure? > > Yes, it's implementation defined. It's probably always a byte on compilers we > use, but I agree we should change the field to be sure. I changed this. We now don't load is_active_ anymore, but last_step_action_ instead. https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter.cc:1797: Label if_debugging(assembler), debugger_done(assembler); On 2016/06/14 06:23:11, rmcilroy wrote: > Please make if_debugging deferred. (It's the only call in this handler, so if > it's deferred the handler won't build a frame unless it enters this branch). Done. However, I think we always build a frame for this bytecode handler.
The CQ bit was checked by yangguo@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/2067503002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Interpreter LGTM once comments are addressed. https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter.cc File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2067503002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter.cc:1797: Label if_debugging(assembler), debugger_done(assembler); On 2016/06/14 09:17:02, Yang wrote: > On 2016/06/14 06:23:11, rmcilroy wrote: > > Please make if_debugging deferred. (It's the only call in this handler, so if > > it's deferred the handler won't build a frame unless it enters this branch). > > Done. However, I think we always build a frame for this bytecode handler. We do on ia32 due to lack of registers, but not on x64 currently. We shouldn't need to on Arm either, but seem to for some reason, I'll look into that at some point. Thanks. https://codereview.chromium.org/2067503002/diff/20001/src/interpreter/interpr... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2067503002/diff/20001/src/interpreter/interpr... src/interpreter/interpreter-assembler.cc:623: Node* InterpreterAssembler::DebugSteppingAcrossAwait() { This is no longer a generic operation that might be useful elsewhere, so I don't think it makes sense to live in InterpreterAssembler any longer. Could you move it to where it is used in SuspendGenerators please. https://codereview.chromium.org/2067503002/diff/20001/src/interpreter/interpr... src/interpreter/interpreter-assembler.cc:629: STATIC_ASSERT(StepFrame > StepNext); Could you assert that StepFrame is the last element in the enum as well?
https://codereview.chromium.org/2067503002/diff/20001/src/interpreter/interpr... File src/interpreter/interpreter-assembler.cc (right): https://codereview.chromium.org/2067503002/diff/20001/src/interpreter/interpr... src/interpreter/interpreter-assembler.cc:623: Node* InterpreterAssembler::DebugSteppingAcrossAwait() { On 2016/06/14 10:00:22, rmcilroy wrote: > This is no longer a generic operation that might be useful elsewhere, so I don't > think it makes sense to live in InterpreterAssembler any longer. Could you move > it to where it is used in SuspendGenerators please. Done. https://codereview.chromium.org/2067503002/diff/20001/src/interpreter/interpr... src/interpreter/interpreter-assembler.cc:629: STATIC_ASSERT(StepFrame > StepNext); On 2016/06/14 10:00:22, rmcilroy wrote: > Could you assert that StepFrame is the last element in the enum as well? Done.
Description was changed from ========== [debugger] fix stepping over await calls for ignition generators. R=*bmeurer@chromium.org, caitpotter88@gmail.com, neis@chromium.org BUG=v8:5099 ========== to ========== [debugger] fix stepping over await calls for ignition generators. R=bmeurer@chromium.org, caitpotter88@gmail.com, neis@chromium.org BUG=v8:5099 ==========
yangguo@chromium.org changed required reviewers: - bmeurer@chromium.org
lgtm
The CQ bit was checked by yangguo@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/2067503002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/2067503002/#ps40001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067503002/40001
Message was sent while issue was closed.
Description was changed from ========== [debugger] fix stepping over await calls for ignition generators. R=bmeurer@chromium.org, caitpotter88@gmail.com, neis@chromium.org BUG=v8:5099 ========== to ========== [debugger] fix stepping over await calls for ignition generators. R=bmeurer@chromium.org, caitpotter88@gmail.com, neis@chromium.org BUG=v8:5099 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [debugger] fix stepping over await calls for ignition generators. R=bmeurer@chromium.org, caitpotter88@gmail.com, neis@chromium.org BUG=v8:5099 ========== to ========== [debugger] fix stepping over await calls for ignition generators. R=bmeurer@chromium.org, caitpotter88@gmail.com, neis@chromium.org BUG=v8:5099 Committed: https://crrev.com/098964544aad30ad9bda91639962e9ff951db09b Cr-Commit-Position: refs/heads/master@{#36964} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/098964544aad30ad9bda91639962e9ff951db09b Cr-Commit-Position: refs/heads/master@{#36964} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
