[liveedit] reimplement frame restarting.

[liveedit] reimplement frame restarting.

Previously, when restarting a frame, we would rewrite all frames
between the debugger activation and the frame to restart to squash
them, and replace the return address with that of a builtin to
leave that rewritten frame, and restart the function by calling it.

We now simply remember the frame to drop to, and upon returning
from the debugger, we check whether to drop the frame, load the
new FP, and restart the function.


R=jgruber@chromium.org, mstarzinger@chromium.org
BUG=v8:5587
Review-Url: https://codereview.chromium.org/2636913002
Cr-Commit-Position: refs/heads/master@{#42725}
Committed: https://chromium.googlesource.com/v8/v8/+/3f47c63ded839194989ba69c3039ea073636ef30

[Michael Starzinger, 2017-01-17 11:53:35]

LGTM on the plumbing outside the debugger. Cool stuff, I like it.

[jgruber, 2017-01-17 13:29:59]

This looks great, much simpler than the previous solution.

lgtm with comments.

https://codereview.chromium.org/2636913002/diff/1/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2636913002/diff/1/src/debug/debug.h#newcode564
src/debug/debug.h:564: // Drop to a lower a frame.
Nit: 'a lower a'.

A diagram or brief description of how frame dropping works would be very helpful
since the code to handle it is pretty scattered (and this function doesn't
actually drop frames as its name suggests).

If I understood correctly, this function marks an upcoming drop, which then
actually takes place once we return from the DebugBreakStub. new_fp_ is only
reset when entering a new DebugBreak.

https://codereview.chromium.org/2636913002/diff/1/src/debug/debug.h#newcode736
src/debug/debug.h:736: // this value will be the hole.
Is the comment about the hole still valid?

https://codereview.chromium.org/2636913002/diff/1/src/debug/debug.h#newcode743
src/debug/debug.h:743: Address new_fp_;
Nit: 'new_fp_' isn't very descriptive, maybe something like frame_drop_fp? Up to
you.

https://codereview.chromium.org/2636913002/diff/1/src/debug/liveedit.cc
File src/debug/liveedit.cc (right):

https://codereview.chromium.org/2636913002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:1301: // and therefore has to be a JavaScript frame.
What about a DCHECK(bottom_frame->type() == JAVA_SCRIPT)?

https://codereview.chromium.org/2636913002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:1311: StackFrame* bottom_frame =
frames[bottom_js_frame_index];
Nit: We could rename bottom_js_frame_index parameter as well.

https://codereview.chromium.org/2636913002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2636913002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:1189: Node* new_fp =
Load(MachineType::TaggedPointer(), new_fp_address);
Is new_fp really a TaggedPointer? Looking at the FrameDropperTrampoline, it
seems like a raw pointer (MachineType::PointerRepresentation()) to me.

https://codereview.chromium.org/2636913002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:1190: Node* null =
IntPtrConstant(static_cast<intptr_t>(0));
Nit: we don't need the static_cast.

[Michael Starzinger, 2017-01-17 14:45:28]

https://codereview.chromium.org/2636913002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2636913002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:1189: Node* new_fp =
Load(MachineType::TaggedPointer(), new_fp_address);
On 2017/01/17 13:29:58, jgruber wrote:
> Is new_fp really a TaggedPointer? Looking at the FrameDropperTrampoline, it
> seems like a raw pointer (MachineType::PointerRepresentation()) to me.

+1, nice catch, I totally missed that!

[Yang, 2017-01-18 07:49:08]

https://codereview.chromium.org/2636913002/diff/1/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2636913002/diff/1/src/debug/debug.h#newcode564
src/debug/debug.h:564: // Drop to a lower a frame.
On 2017/01/17 13:29:58, jgruber wrote:
> Nit: 'a lower a'.
> 
> A diagram or brief description of how frame dropping works would be very
helpful
> since the code to handle it is pretty scattered (and this function doesn't
> actually drop frames as its name suggests).
> 
> If I understood correctly, this function marks an upcoming drop, which then
> actually takes place once we return from the DebugBreakStub. new_fp_ is only
> reset when entering a new DebugBreak.

Done. Added comment.

https://codereview.chromium.org/2636913002/diff/1/src/debug/debug.h#newcode736
src/debug/debug.h:736: // this value will be the hole.
On 2017/01/17 13:29:58, jgruber wrote:
> Is the comment about the hole still valid?

Done.

https://codereview.chromium.org/2636913002/diff/1/src/debug/debug.h#newcode743
src/debug/debug.h:743: Address new_fp_;
On 2017/01/17 13:29:58, jgruber wrote:
> Nit: 'new_fp_' isn't very descriptive, maybe something like frame_drop_fp? Up
to
> you.

renamed to restart_fp_

https://codereview.chromium.org/2636913002/diff/1/src/debug/liveedit.cc
File src/debug/liveedit.cc (right):

https://codereview.chromium.org/2636913002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:1301: // and therefore has to be a JavaScript frame.
On 2017/01/17 13:29:58, jgruber wrote:
> What about a DCHECK(bottom_frame->type() == JAVA_SCRIPT)?

Done.

https://codereview.chromium.org/2636913002/diff/1/src/debug/liveedit.cc#newco...
src/debug/liveedit.cc:1311: StackFrame* bottom_frame =
frames[bottom_js_frame_index];
On 2017/01/17 13:29:58, jgruber wrote:
> Nit: We could rename bottom_js_frame_index parameter as well.

Folded all of this into a new function "ScheduleFrameRestart"

https://codereview.chromium.org/2636913002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2636913002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:1189: Node* new_fp =
Load(MachineType::TaggedPointer(), new_fp_address);
On 2017/01/17 14:45:27, Michael Starzinger wrote:
> On 2017/01/17 13:29:58, jgruber wrote:
> > Is new_fp really a TaggedPointer? Looking at the FrameDropperTrampoline, it
> > seems like a raw pointer (MachineType::PointerRepresentation()) to me.
> 
> +1, nice catch, I totally missed that!

Done.

https://codereview.chromium.org/2636913002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:1190: Node* null =
IntPtrConstant(static_cast<intptr_t>(0));
On 2017/01/17 13:29:58, jgruber wrote:
> Nit: we don't need the static_cast.

Done.

[jgruber, 2017-01-18 09:10:38]

Thanks, still lgtm.

[Yang, 2017-01-19 12:07:47]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-01-19 12:07:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2636913002/#ps120001
(title: "small fixes")

[commit-bot: I haz the power, 2017-01-19 12:07:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 12:31:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 12:31:01]

Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15968)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Yang, 2017-01-20 06:14:46]

On 2017/01/19 12:31:01, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15968)
>   v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

Fixed the issue with optimized code. Now whenever we restart an optimized frame,
we deopt it and let the deoptimizer handle jumping into the frame dropper
trampoline.

[Yang, 2017-01-20 06:14:53]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 06:14:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 06:16:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 06:16:47]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19573)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[Yang, 2017-01-20 06:46:39]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 06:46:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 07:10:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 07:10:10]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19644)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21194)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[Yang, 2017-01-24 11:55:26]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 11:55:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 12:14:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 12:14:36]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Yang, 2017-01-24 14:08:10]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 14:08:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 14:42:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 14:42:02]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-01-24 14:44:17]

On 2017/01/20 06:14:46, Yang wrote:
> On 2017/01/19 12:31:01, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15968)
> >   v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
> 
> Fixed the issue with optimized code. Now whenever we restart an optimized
frame,
> we deopt it and let the deoptimizer handle jumping into the frame dropper
> trampoline.

Michael, could you give the changes to TF a lool before I land this? Thanks.

[Yang, 2017-01-26 08:30:04]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-01-26 08:30:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2636913002/#ps220001
(title: "fix operator property")

[commit-bot: I haz the power, 2017-01-26 08:30:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 09:05:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 09:05:07]

Failed to apply patch for src/external-reference-table.cc:
While running git apply --index -p1;
  error: patch failed: src/external-reference-table.cc:257
  error: src/external-reference-table.cc: patch does not apply

Patch:       src/external-reference-table.cc
Index: src/external-reference-table.cc
diff --git a/src/external-reference-table.cc b/src/external-reference-table.cc
index
57dc09ce3c3aa2635ac53484246fc95a355940e8..8405396cf42846e3fdd6deb1f254413a47637ff0
100644
--- a/src/external-reference-table.cc
+++ b/src/external-reference-table.cc
@@ -257,8 +257,6 @@ void ExternalReferenceTable::AddReferences(Isolate* isolate)
{
       "Isolate::promise_hook_address()");
 
   // Debug addresses
-  Add(ExternalReference::debug_after_break_target_address(isolate).address(),
-      "Debug::after_break_target_address()");
   Add(ExternalReference::debug_is_active_address(isolate).address(),
       "Debug::is_active_address()");
  
Add(ExternalReference::debug_hook_on_function_call_address(isolate).address(),
@@ -267,6 +265,8 @@ void ExternalReferenceTable::AddReferences(Isolate* isolate)
{
       "Debug::step_in_enabled_address()");
   Add(ExternalReference::debug_suspended_generator_address(isolate).address(),
       "Debug::step_suspended_generator_address()");
+  Add(ExternalReference::debug_restart_fp_address(isolate).address(),
+      "Debug::restart_fp_address()");
 
 #ifndef V8_INTERPRETED_REGEXP
   Add(ExternalReference::re_case_insensitive_compare_uc16(isolate).address(),

[Yang, 2017-01-27 06:55:16]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-01-27 06:55:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2636913002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-27 06:55:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 07:31:06]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1485500116390140,
"parent_rev": "34bee46b0be6afc06f254ec0debd4b868443a649", "commit_rev":
"3f47c63ded839194989ba69c3039ea073636ef30"}

[commit-bot: I haz the power, 2017-01-27 07:31:11]

Description was changed from

==========
[liveedit] reimplement frame restarting.

Previously, when restarting a frame, we would rewrite all frames
between the debugger activation and the frame to restart to squash
them, and replace the return address with that of a builtin to
leave that rewritten frame, and restart the function by calling it.

We now simply remember the frame to drop to, and upon returning
from the debugger, we check whether to drop the frame, load the
new FP, and restart the function.


R=jgruber@chromium.org, mstarzinger@chromium.org
BUG=v8:5587
==========

to

==========
[liveedit] reimplement frame restarting.

Previously, when restarting a frame, we would rewrite all frames
between the debugger activation and the frame to restart to squash
them, and replace the return address with that of a builtin to
leave that rewritten frame, and restart the function by calling it.

We now simply remember the frame to drop to, and upon returning
from the debugger, we check whether to drop the frame, load the
new FP, and restart the function.


R=jgruber@chromium.org, mstarzinger@chromium.org
BUG=v8:5587

Review-Url: https://codereview.chromium.org/2636913002
Cr-Commit-Position: refs/heads/master@{#42725}
Committed:
https://chromium.googlesource.com/v8/v8/+/3f47c63ded839194989ba69c3039ea07363...
==========

[commit-bot: I haz the power, 2017-01-27 07:31:13]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/v8/v8/+/3f47c63ded839194989ba69c3039ea07363...