Reland "[debug] only break on entry when immediately called from JS"

This reverts commit 0bd19ddb.

TBR=szuend@chromium.org

Change-Id: I86bc9409cb809ff978a1104be79bbbe4b87f85e5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1767996Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63358}

include/v8.h

@@ -129,6 +129,7 @@ class PropertyCallbackArguments;
 class FunctionCallbackArguments;
 class GlobalHandles;
 class ScopedExternalStringLock;
+class ThreadLocalTop;
 
 namespace wasm {
 class NativeModule;
@@ -7664,6 +7665,9 @@ class V8_EXPORT Isolate {
    private:
     internal::Isolate* const isolate_;
     internal::MicrotaskQueue* const microtask_queue_;
+    internal::Address previous_stack_height_;
+
+    friend class internal::ThreadLocalTop;
   };
 
   /**

src/api/api.cc

@@ -262,7 +262,7 @@ void CheckMicrotasksScopesConsistency(i::MicrotaskQueue* microtask_queue) {
 template <bool do_callback>
 class CallDepthScope {
  public:
-  explicit CallDepthScope(i::Isolate* isolate, Local<Context> context)
+  CallDepthScope(i::Isolate* isolate, Local<Context> context)
       : isolate_(isolate),
         context_(context),
         escaped_(false),
@@ -273,7 +273,7 @@ class CallDepthScope {
                                      ? i::InterruptsScope::kRunInterrupts
                                      : i::InterruptsScope::kPostponeInterrupts)
                               : i::InterruptsScope::kNoop) {
-    isolate_->handle_scope_implementer()->IncrementCallDepth();
+    isolate_->thread_local_top()->IncrementCallDepth(this);
     isolate_->set_next_v8_call_is_safe_for_termination(false);
     if (!context.IsEmpty()) {
       i::Handle<i::Context> env = Utils::OpenHandle(*context);
@@ -297,7 +297,7 @@ class CallDepthScope {
       i::Handle<i::Context> env = Utils::OpenHandle(*context_);
       microtask_queue = env->native_context().microtask_queue();
     }
-    if (!escaped_) isolate_->handle_scope_implementer()->DecrementCallDepth();
+    if (!escaped_) isolate_->thread_local_top()->DecrementCallDepth(this);
     if (do_callback) isolate_->FireCallCompletedCallback(microtask_queue);
 // TODO(jochen): This should be #ifdef DEBUG
 #ifdef V8_CHECK_MICROTASKS_SCOPES_CONSISTENCY
@@ -309,11 +309,10 @@ class CallDepthScope {
   void Escape() {
     DCHECK(!escaped_);
     escaped_ = true;
-    auto handle_scope_implementer = isolate_->handle_scope_implementer();
-    handle_scope_implementer->DecrementCallDepth();
-    bool clear_exception =
-        handle_scope_implementer->CallDepthIsZero() &&
-        isolate_->thread_local_top()->try_catch_handler_ == nullptr;
+    auto thread_local_top = isolate_->thread_local_top();
+    thread_local_top->DecrementCallDepth(this);
+    bool clear_exception = thread_local_top->CallDepthIsZero() &&
+                           thread_local_top->try_catch_handler_ == nullptr;
     isolate_->OptionalRescheduleException(clear_exception);
   }
 
@@ -324,6 +323,12 @@ class CallDepthScope {
   bool do_callback_;
   bool safe_for_termination_;
   i::InterruptsScope interrupts_scope_;
+  i::Address previous_stack_height_;
+
+  friend class i::ThreadLocalTop;
+
+  DISALLOW_NEW_AND_DELETE()
+  DISALLOW_COPY_AND_ASSIGN(CallDepthScope);
 };
 
 }  // namespace
@@ -8068,13 +8073,13 @@ Isolate::SuppressMicrotaskExecutionScope::SuppressMicrotaskExecutionScope(
     Isolate* isolate)
     : isolate_(reinterpret_cast<i::Isolate*>(isolate)),
       microtask_queue_(isolate_->default_microtask_queue()) {
-  isolate_->handle_scope_implementer()->IncrementCallDepth();
+  isolate_->thread_local_top()->IncrementCallDepth(this);
   microtask_queue_->IncrementMicrotasksSuppressions();
 }
 
 Isolate::SuppressMicrotaskExecutionScope::~SuppressMicrotaskExecutionScope() {
   microtask_queue_->DecrementMicrotasksSuppressions();
-  isolate_->handle_scope_implementer()->DecrementCallDepth();
+  isolate_->thread_local_top()->DecrementCallDepth(this);
 }
 
 Isolate::SafeForTerminationScope::SafeForTerminationScope(v8::Isolate* isolate)

src/api/api.h

@@ -357,7 +357,6 @@ class HandleScopeImplementer {
   explicit HandleScopeImplementer(Isolate* isolate)
       : isolate_(isolate),
         spare_(nullptr),
-        call_depth_(0),
         last_handle_before_deferred_block_(nullptr) {}
 
   ~HandleScopeImplementer() { DeleteArray(spare_); }
@@ -376,11 +375,6 @@ class HandleScopeImplementer {
   inline internal::Address* GetSpareOrNewBlock();
   inline void DeleteExtensions(internal::Address* prev_limit);
 
-  // Call depth represents nested v8 api calls.
-  inline void IncrementCallDepth() { call_depth_++; }
-  inline void DecrementCallDepth() { call_depth_--; }
-  inline bool CallDepthIsZero() { return call_depth_ == 0; }
-
   inline void EnterContext(Context context);
   inline void LeaveContext();
   inline bool LastEnteredContextWas(Context context);
@@ -417,7 +411,6 @@ class HandleScopeImplementer {
     saved_contexts_.detach();
     spare_ = nullptr;
     last_handle_before_deferred_block_ = nullptr;
-    call_depth_ = 0;
   }
 
   void Free() {
@@ -434,7 +427,7 @@ class HandleScopeImplementer {
       DeleteArray(spare_);
       spare_ = nullptr;
     }
-    DCHECK_EQ(call_depth_, 0);
+    DCHECK(isolate_->thread_local_top()->CallDepthIsZero());
   }
 
   void BeginDeferredScope();
@@ -454,8 +447,6 @@ class HandleScopeImplementer {
   // Used as a stack to keep track of saved contexts.
   DetachableVector<Context> saved_contexts_;
   Address* spare_;
-  int call_depth_;
-
   Address* last_handle_before_deferred_block_;
   // This is only used for threading support.
   HandleScopeData handle_scope_data_;

src/debug/debug.h

@@ -375,6 +375,8 @@ class V8_EXPORT_PRIVATE Debug {
     return thread_local_.break_on_next_function_call_;
   }
 
+  inline bool break_disabled() const { return break_disabled_; }
+
   DebugFeatureTracker* feature_tracker() { return &feature_tracker_; }
 
   // For functions in which we cannot set a break point, use a canonical
@@ -399,7 +401,6 @@ class V8_EXPORT_PRIVATE Debug {
     return is_suppressed_ || !is_active_ ||
            isolate_->debug_execution_mode() == DebugInfo::kSideEffects;
   }
-  inline bool break_disabled() const { return break_disabled_; }
 
   void clear_suspended_generator() {
     thread_local_.suspended_generator_ = Smi::kZero;

src/execution/isolate.cc

@@ -2023,7 +2023,7 @@ void Isolate::CancelScheduledExceptionFromTryCatch(v8::TryCatch* handler) {
     DCHECK_EQ(scheduled_exception(),
               ReadOnlyRoots(heap()).termination_exception());
     // Clear termination once we returned from all V8 frames.
-    if (handle_scope_implementer()->CallDepthIsZero()) {
+    if (thread_local_top()->CallDepthIsZero()) {
       thread_local_top()->external_caught_exception_ = false;
       clear_scheduled_exception();
     }
@@ -4250,7 +4250,7 @@ void Isolate::RemoveCallCompletedCallback(CallCompletedCallback callback) {
 }
 
 void Isolate::FireCallCompletedCallback(MicrotaskQueue* microtask_queue) {
-  if (!handle_scope_implementer()->CallDepthIsZero()) return;
+  if (!thread_local_top()->CallDepthIsZero()) return;
 
   bool run_microtasks =
       microtask_queue && microtask_queue->size() &&

src/execution/thread-local-top.cc

@@ -26,5 +26,15 @@ void ThreadLocalTop::Free() {
   while (promise_on_stack_) isolate_->PopPromise();
 }
 
+#if defined(USE_SIMULATOR)
+void ThreadLocalTop::StoreCurrentStackPosition() {
+  last_api_entry_ = simulator_->get_sp();
+}
+#elif defined(V8_USE_ADDRESS_SANITIZER)
+void ThreadLocalTop::StoreCurrentStackPosition() {
+  last_api_entry_ = reinterpret_cast<Address>(GetCurrentStackPosition());
+}
+#endif
+
 }  // namespace internal
 }  // namespace v8

src/execution/thread-local-top.h

@@ -8,6 +8,7 @@
 #include "src/common/globals.h"
 #include "src/execution/thread-id.h"
 #include "src/objects/contexts.h"
+#include "src/utils/utils.h"
 
 namespace v8 {
 
@@ -25,7 +26,7 @@ class ThreadLocalTop {
   // TODO(all): This is not particularly beautiful. We should probably
   // refactor this to really consist of just Addresses and 32-bit
   // integer fields.
-  static constexpr uint32_t kSizeInBytes = 23 * kSystemPointerSize;
+  static constexpr uint32_t kSizeInBytes = 24 * kSystemPointerSize;
 
   // Does early low-level initialization that does not depend on the
   // isolate being present.
@@ -56,6 +57,31 @@ class ThreadLocalTop {
         v8::TryCatch::JSStackComparableAddress(try_catch_handler_));
   }
 
+  // Call depth represents nested v8 api calls. Instead of storing the nesting
+  // level as an integer, we store the stack height of the last API entry. This
+  // additional information is used when we decide whether to trigger a debug
+  // break at a function entry.
+  template <typename Scope>
+  void IncrementCallDepth(Scope* stack_allocated_scope) {
+    stack_allocated_scope->previous_stack_height_ = last_api_entry_;
+#if defined(USE_SIMULATOR) || defined(V8_USE_ADDRESS_SANITIZER)
+    StoreCurrentStackPosition();
+#else
+    last_api_entry_ = reinterpret_cast<i::Address>(stack_allocated_scope);
+#endif
+  }
+
+#if defined(USE_SIMULATOR) || defined(V8_USE_ADDRESS_SANITIZER)
+  void StoreCurrentStackPosition();
+#endif
+
+  template <typename Scope>
+  void DecrementCallDepth(Scope* stack_allocated_scope) {
+    last_api_entry_ = stack_allocated_scope->previous_stack_height_;
+  }
+
+  bool CallDepthIsZero() const { return last_api_entry_ == kNullAddress; }
+
   void Free();
 
   Isolate* isolate_ = nullptr;
@@ -77,6 +103,8 @@ class ThreadLocalTop {
   Address pending_handler_fp_ = kNullAddress;
   Address pending_handler_sp_ = kNullAddress;
 
+  Address last_api_entry_ = kNullAddress;
+
   // Communication channel between Isolate::Throw and message consumers.
   Object pending_message_obj_;
   bool rethrowing_message_ = false;

src/runtime/runtime-debug.cc

@@ -115,10 +115,17 @@ RUNTIME_FUNCTION(Runtime_DebugBreakAtEntry) {
   DCHECK(function->shared().HasDebugInfo());
   DCHECK(function->shared().GetDebugInfo().BreakAtEntry());
 
-  // Get the top-most JavaScript frame.
+  // Get the top-most JavaScript frame. This is the debug target function.
   JavaScriptFrameIterator it(isolate);
   DCHECK_EQ(*function, it.frame()->function());
+  // Check whether the next JS frame is closer than the last API entry.
+  // if yes, then the call to the debug target came from JavaScript. Otherwise,
+  // the call to the debug target came from API. Do not break for the latter.
+  it.Advance();
+  if (!it.done() &&
+      it.frame()->fp() < isolate->thread_local_top()->last_api_entry_) {
     isolate->debug()->Break(it.frame(), function);
+  }
 
   return ReadOnlyRoots(isolate).undefined_value();
 }

test/cctest/test-debug.cc

@@ -1097,14 +1097,15 @@ TEST(BreakPointApiFunction) {
   ExpectInt32("f()", 2);
   CHECK_EQ(2, break_point_hit_count);
 
+  // Direct call through API does not trigger breakpoint.
   function->Call(env.local(), v8::Undefined(env->GetIsolate()), 0, nullptr)
       .ToLocalChecked();
-  CHECK_EQ(3, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   ExpectInt32("f()", 2);
-  CHECK_EQ(3, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();
@@ -1137,13 +1138,14 @@ TEST(BreakPointApiConstructor) {
   CompileRun("new f()");
   CHECK_EQ(2, break_point_hit_count);
 
+  // Direct call through API does not trigger breakpoint.
   function->NewInstance(env.local()).ToLocalChecked();
-  CHECK_EQ(3, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("new f()");
-  CHECK_EQ(3, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();
@@ -1188,15 +1190,16 @@ TEST(BreakPointApiGetter) {
   // Run with breakpoint.
   bp = SetBreakPoint(function, 0);
   CompileRun("get_wrapper(o, 'f')");
-  CHECK_EQ(1, break_point_hit_count);
+  CHECK_EQ(0, break_point_hit_count);
 
   CompileRun("o.f");
-  CHECK_EQ(2, break_point_hit_count);
+  CHECK_EQ(1, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("get_wrapper(o, 'f', 2)");
-  CHECK_EQ(2, break_point_hit_count);
+  CompileRun("o.f");
+  CHECK_EQ(1, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();
@@ -1245,12 +1248,12 @@ TEST(BreakPointApiSetter) {
   CHECK_EQ(1, break_point_hit_count);
 
   CompileRun("set_wrapper(o, 'f', 2)");
-  CHECK_EQ(2, break_point_hit_count);
+  CHECK_EQ(1, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("o.f = 3");
-  CHECK_EQ(2, break_point_hit_count);
+  CHECK_EQ(1, break_point_hit_count);
 
   // === Test API builtin as setter, with condition ===
   break_point_hit_count = 0;
@@ -1261,15 +1264,16 @@ TEST(BreakPointApiSetter) {
   CHECK_EQ(0, break_point_hit_count);
 
   CompileRun("set_wrapper(o, 'f', 3)");
-  CHECK_EQ(1, break_point_hit_count);
+  CHECK_EQ(0, break_point_hit_count);
 
   CompileRun("o.f = 3");
-  CHECK_EQ(2, break_point_hit_count);
+  CHECK_EQ(1, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("set_wrapper(o, 'f', 2)");
-  CHECK_EQ(2, break_point_hit_count);
+  CompileRun("o.f = 3");
+  CHECK_EQ(1, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();