[debugger] account for inlined functions when stepping.

- Remove obsolete BreakLocatorType.
- Perform PrepareStepOnThrow after OnException event, in case stepping
  was scheduled in the exception event.
- Use frame count instead of frame pointer for stepping. Frame pointer
  is not reliable due to possible deopts.
- Consistently check for inlined functions in inlined frames.
- Use SharedFunctionInfo in FloodWithOneshot and EnsureDebugInfo.

R=jgruber@chromium.org
BUG=v8:5901

Review-Url: https://codereview.chromium.org/2664793002
Cr-Commit-Position: refs/heads/master@{#42878}

src/debug/debug.h

@@ -50,10 +50,6 @@ enum ExceptionBreakType {
 };
 
 
-// Type of exception break.
-enum BreakLocatorType { ALL_BREAK_LOCATIONS, CALLS_AND_RETURNS };
-
-
 // The different types of breakpoint position alignments.
 // Must match Debug.BreakPositionAlignment in debug.js
 enum BreakPositionAlignment {
@@ -122,8 +118,7 @@ class BreakLocation {
 class BreakIterator {
  public:
   static std::unique_ptr<BreakIterator> GetIterator(
-      Handle<DebugInfo> debug_info, Handle<AbstractCode> abstract_code,
-      BreakLocatorType type = ALL_BREAK_LOCATIONS);
+      Handle<DebugInfo> debug_info, Handle<AbstractCode> abstract_code);
 
   virtual ~BreakIterator() {}
 
@@ -145,8 +140,7 @@ class BreakIterator {
   virtual void SetDebugBreak() = 0;
 
  protected:
-  explicit BreakIterator(Handle<DebugInfo> debug_info,
-                         BreakLocatorType break_locator_type);
+  explicit BreakIterator(Handle<DebugInfo> debug_info);
 
   int BreakIndexFromPosition(int position, BreakPositionAlignment alignment);
 
@@ -156,7 +150,6 @@ class BreakIterator {
   int break_index_;
   int position_;
   int statement_position_;
-  BreakLocatorType break_locator_type_;
 
  private:
   DisallowHeapAllocation no_gc_;
@@ -165,7 +158,7 @@ class BreakIterator {
 
 class CodeBreakIterator : public BreakIterator {
  public:
-  CodeBreakIterator(Handle<DebugInfo> debug_info, BreakLocatorType type);
+  explicit CodeBreakIterator(Handle<DebugInfo> debug_info);
   ~CodeBreakIterator() override {}
 
   BreakLocation GetBreakLocation() override;
@@ -184,7 +177,7 @@ class CodeBreakIterator : public BreakIterator {
   }
 
  private:
-  int GetModeMask(BreakLocatorType type);
+  int GetModeMask();
   DebugBreakType GetDebugBreakType();
 
   RelocInfo::Mode rmode() { return reloc_iterator_.rinfo()->rmode(); }
@@ -197,8 +190,7 @@ class CodeBreakIterator : public BreakIterator {
 
 class BytecodeArrayBreakIterator : public BreakIterator {
  public:
-  BytecodeArrayBreakIterator(Handle<DebugInfo> debug_info,
-                             BreakLocatorType type);
+  explicit BytecodeArrayBreakIterator(Handle<DebugInfo> debug_info);
   ~BytecodeArrayBreakIterator() override {}
 
   BreakLocation GetBreakLocation() override;
@@ -357,11 +349,8 @@ class Debug {
 
   void SetDebugDelegate(debug::DebugDelegate* delegate);
 
-  // Returns whether the operation succeeded. Compilation can only be triggered
-  // if a valid closure is passed as the second argument, otherwise the shared
-  // function needs to be compiled already.
-  bool EnsureDebugInfo(Handle<SharedFunctionInfo> shared,
-                       Handle<JSFunction> function);
+  // Returns whether the operation succeeded.
+  bool EnsureDebugInfo(Handle<SharedFunctionInfo> shared);
   void CreateDebugInfo(Handle<SharedFunctionInfo> shared);
   static Handle<DebugInfo> GetDebugInfo(Handle<SharedFunctionInfo> shared);
 
@@ -466,6 +455,9 @@ class Debug {
     thread_local_.break_id_ = ++thread_local_.break_count_;
   }
 
+  // Return the number of virtual frames below debugger entry.
+  int CurrentFrameCount();
+
   inline bool ignore_events() const {
     return is_suppressed_ || !is_active_ || isolate_->needs_side_effect_check();
   }
@@ -492,7 +484,6 @@ class Debug {
     return !event_listener_.is_null() && !event_listener_->IsForeign();
   }
 
-  bool IsBlackboxed(SharedFunctionInfo* shared);
   bool IsExceptionBlackboxed(bool uncaught);
 
   void OnException(Handle<Object> exception, Handle<Object> promise);
@@ -526,8 +517,7 @@ class Debug {
   // Clear all code from instrumentation.
   void ClearAllBreakPoints();
   // Instrument a function with one-shots.
-  void FloodWithOneShot(Handle<JSFunction> function,
-                        BreakLocatorType type = ALL_BREAK_LOCATIONS);
+  void FloodWithOneShot(Handle<SharedFunctionInfo> function);
   // Clear all one-shot instrumentations, but restore break points.
   void ClearOneShot();
 
@@ -607,10 +597,10 @@ class Debug {
     int last_statement_position_;
 
     // Frame pointer from last step next or step frame action.
-    Address last_fp_;
+    int last_frame_count_;
 
     // Frame pointer of the target frame we want to arrive at.
-    Address target_fp_;
+    int target_frame_count_;
 
     // Value of the accumulator at the point of entering the debugger.
     Object* return_value_;

src/frames.cc

@@ -954,6 +954,16 @@ void JavaScriptFrame::GetFunctions(List<SharedFunctionInfo*>* functions) const {
   functions->Add(function()->shared());
 }
 
+void JavaScriptFrame::GetFunctions(
+    List<Handle<SharedFunctionInfo>>* functions) const {
+  DCHECK(functions->length() == 0);
+  List<SharedFunctionInfo*> raw_functions;
+  GetFunctions(&raw_functions);
+  for (const auto& raw_function : raw_functions) {
+    functions->Add(Handle<SharedFunctionInfo>(raw_function));
+  }
+}
+
 void JavaScriptFrame::Summarize(List<FrameSummary>* functions,
                                 FrameSummary::Mode mode) const {
   DCHECK(functions->length() == 0);

src/frames.h

@@ -1029,6 +1029,8 @@ class JavaScriptFrame : public StandardFrame {
   // Return a list with {SharedFunctionInfo} objects of this frame.
   virtual void GetFunctions(List<SharedFunctionInfo*>* functions) const;
 
+  void GetFunctions(List<Handle<SharedFunctionInfo>>* functions) const;
+
   // Lookup exception handler for current {pc}, returns -1 if none found. Also
   // returns data associated with the handler site specific to the frame type:
   //  - OptimizedFrame  : Data is the stack slot count of the entire frame.

test/cctest/test-debug.cc

@@ -6456,14 +6456,13 @@ TEST(BreakLocationIterator) {
   Handle<i::SharedFunctionInfo> shared(function->shared());
 
   EnableDebugger(isolate);
-  CHECK(i_isolate->debug()->EnsureDebugInfo(shared, function));
+  CHECK(i_isolate->debug()->EnsureDebugInfo(shared));
 
   Handle<i::DebugInfo> debug_info(shared->GetDebugInfo());
   Handle<i::AbstractCode> abstract_code(shared->abstract_code());
 
   {
-    auto iterator = i::BreakIterator::GetIterator(debug_info, abstract_code,
-                                                  i::ALL_BREAK_LOCATIONS);
+    auto iterator = i::BreakIterator::GetIterator(debug_info, abstract_code);
     CHECK(iterator->GetBreakLocation().IsDebuggerStatement());
     CHECK_EQ(17, iterator->GetBreakLocation().position());
     iterator->Next();
@@ -6482,18 +6481,6 @@ TEST(BreakLocationIterator) {
     CHECK(iterator->Done());
   }
 
-  {
-    auto iterator = i::BreakIterator::GetIterator(debug_info, abstract_code,
-                                                  i::CALLS_AND_RETURNS);
-    CHECK(iterator->GetBreakLocation().IsCall());
-    CHECK_EQ(32, iterator->GetBreakLocation().position());
-    iterator->Next();
-    CHECK(iterator->GetBreakLocation().IsReturn());
-    CHECK_EQ(60, iterator->GetBreakLocation().position());
-    iterator->Next();
-    CHECK(iterator->Done());
-  }
-
   DisableDebugger(isolate);
 }
 

test/debugger/regress/regress-5901-1.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --ignition --turbo
+
+function f() {
+  throw new Error();
+}
+
+function g() {
+  try {
+    f();
+  } catch (e) {
+    return 1;  // Break
+  }
+}
+
+function h() {
+  return g();
+}
+
+h();
+h();
+
+var Debug = debug.Debug;
+var step_count = 0;
+var exception = null;
+
+function listener(event, exec_state, event_data, data) {
+  if (event == Debug.DebugEvent.Exception) {
+    step_count++;
+    exec_state.prepareStep(Debug.StepAction.StepNext);
+  } else if (event == Debug.DebugEvent.Break) {
+    step_count++;
+    try {
+      assertTrue(exec_state.frame().sourceLineText().includes('Break'));
+    } catch (e) {
+      exception = e;
+      print(e);
+    }
+  }
+}
+
+Debug.setListener(listener);
+Debug.setBreakOnException();
+% OptimizeFunctionOnNextCall(h);
+h();
+Debug.setListener(null);
+assertNull(exception);

test/debugger/regress/regress-5901-2.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+function f() {
+  debugger;
+  return 1;
+}
+
+function g() {
+  return f();
+}  // Break
+
+function h() {
+  return g();
+}
+
+h();
+h();
+
+var Debug = debug.Debug;
+var step_count = 0;
+var exception = null;
+
+function listener(event, exec_state, event_data, data) {
+  if (event != Debug.DebugEvent.Break) return;
+  try {
+    if (step_count == 0) {
+      exec_state.prepareStep(Debug.StepAction.StepOut);
+    } else {
+      assertTrue(exec_state.frame().sourceLineText().includes('Break'));
+    }
+    step_count++;
+  } catch (e) {
+    exception = e;
+    print(e);
+  }
+}
+
+Debug.setListener(listener);
+% OptimizeFunctionOnNextCall(h);
+h();
+Debug.setListener(null);
+assertNull(exception);
+assertEquals(2, step_count);