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

This reverts commit e66cee7e9e.

Reason for revert: Speculative revert for https://ci.chromium.org/p/chromium/builders/try/linux-rel/173349

Original change's description:
> [debug] only break on entry when immediately called from JS
> 
> When we break on function entry, check whether the target function is being
> called from JS after entering V8 through V8's API. We implement this by
> keeping track of the stack height when we enter V8 through the API, and compare
> the caller JS frame's stack height with that.
> 
> R=​szuend@chromium.org
> 
> Bug: chromium:991217, chromium:992406
> Change-Id: I258ad9cef11fe0ef48de6fd5055790792fd0ec0c
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1762298
> Commit-Queue: Yang Guo <yangguo@chromium.org>
> Reviewed-by: Simon Zünd <szuend@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#63331}

TBR=yangguo@chromium.org,szuend@chromium.org

Change-Id: I4bfb42f7ce1484807696048a09609f14113d10f4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:991217, chromium:992406
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1762525
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63341}

include/v8.h

@@ -129,7 +129,6 @@ class PropertyCallbackArguments;
 class FunctionCallbackArguments;
 class GlobalHandles;
 class ScopedExternalStringLock;
-class ThreadLocalTop;
 
 namespace wasm {
 class NativeModule;
@@ -7665,9 +7664,6 @@ 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:
-  CallDepthScope(i::Isolate* isolate, Local<Context> context)
+  explicit 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_->thread_local_top()->IncrementCallDepth(this);
+    isolate_->handle_scope_implementer()->IncrementCallDepth();
     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_->thread_local_top()->DecrementCallDepth(this);
+    if (!escaped_) isolate_->handle_scope_implementer()->DecrementCallDepth();
     if (do_callback) isolate_->FireCallCompletedCallback(microtask_queue);
 // TODO(jochen): This should be #ifdef DEBUG
 #ifdef V8_CHECK_MICROTASKS_SCOPES_CONSISTENCY
@@ -309,10 +309,11 @@ class CallDepthScope {
   void Escape() {
     DCHECK(!escaped_);
     escaped_ = true;
-    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;
+    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;
     isolate_->OptionalRescheduleException(clear_exception);
   }
 
@@ -323,12 +324,6 @@ 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
@@ -8073,13 +8068,13 @@ Isolate::SuppressMicrotaskExecutionScope::SuppressMicrotaskExecutionScope(
     Isolate* isolate)
     : isolate_(reinterpret_cast<i::Isolate*>(isolate)),
       microtask_queue_(isolate_->default_microtask_queue()) {
-  isolate_->thread_local_top()->IncrementCallDepth(this);
+  isolate_->handle_scope_implementer()->IncrementCallDepth();
   microtask_queue_->IncrementMicrotasksSuppressions();
 }
 
 Isolate::SuppressMicrotaskExecutionScope::~SuppressMicrotaskExecutionScope() {
   microtask_queue_->DecrementMicrotasksSuppressions();
-  isolate_->thread_local_top()->DecrementCallDepth(this);
+  isolate_->handle_scope_implementer()->DecrementCallDepth();
 }
 
 Isolate::SafeForTerminationScope::SafeForTerminationScope(v8::Isolate* isolate)

src/api/api.h

@@ -357,6 +357,7 @@ class HandleScopeImplementer {
   explicit HandleScopeImplementer(Isolate* isolate)
       : isolate_(isolate),
         spare_(nullptr),
+        call_depth_(0),
         last_handle_before_deferred_block_(nullptr) {}
 
   ~HandleScopeImplementer() { DeleteArray(spare_); }
@@ -375,6 +376,11 @@ 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);
@@ -411,6 +417,7 @@ class HandleScopeImplementer {
     saved_contexts_.detach();
     spare_ = nullptr;
     last_handle_before_deferred_block_ = nullptr;
+    call_depth_ = 0;
   }
 
   void Free() {
@@ -427,7 +434,7 @@ class HandleScopeImplementer {
       DeleteArray(spare_);
       spare_ = nullptr;
     }
-    DCHECK(isolate_->thread_local_top()->CallDepthIsZero());
+    DCHECK_EQ(call_depth_, 0);
   }
 
   void BeginDeferredScope();
@@ -447,6 +454,8 @@ 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,8 +375,6 @@ 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
@@ -401,6 +399,7 @@ 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 (thread_local_top()->CallDepthIsZero()) {
+    if (handle_scope_implementer()->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 (!thread_local_top()->CallDepthIsZero()) return;
+  if (!handle_scope_implementer()->CallDepthIsZero()) return;
 
   bool run_microtasks =
       microtask_queue && microtask_queue->size() &&

src/execution/thread-local-top.cc

@@ -26,15 +26,5 @@ 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,7 +8,6 @@
 #include "src/common/globals.h"
 #include "src/execution/thread-id.h"
 #include "src/objects/contexts.h"
-#include "src/utils/utils.h"
 
 namespace v8 {
 
@@ -26,7 +25,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 = 24 * kSystemPointerSize;
+  static constexpr uint32_t kSizeInBytes = 23 * kSystemPointerSize;
 
   // Does early low-level initialization that does not depend on the
   // isolate being present.
@@ -57,31 +56,6 @@ 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;
@@ -103,8 +77,6 @@ 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,17 +115,10 @@ RUNTIME_FUNCTION(Runtime_DebugBreakAtEntry) {
   DCHECK(function->shared().HasDebugInfo());
   DCHECK(function->shared().GetDebugInfo().BreakAtEntry());
 
-  // Get the top-most JavaScript frame. This is the debug target function.
+  // Get the top-most JavaScript frame.
   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);
-  }
+  isolate->debug()->Break(it.frame(), function);
 
   return ReadOnlyRoots(isolate).undefined_value();
 }

test/cctest/test-debug.cc

@@ -1097,15 +1097,14 @@ 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(2, break_point_hit_count);
+  CHECK_EQ(3, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   ExpectInt32("f()", 2);
-  CHECK_EQ(2, break_point_hit_count);
+  CHECK_EQ(3, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();
@@ -1138,14 +1137,13 @@ 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(2, break_point_hit_count);
+  CHECK_EQ(3, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("new f()");
-  CHECK_EQ(2, break_point_hit_count);
+  CHECK_EQ(3, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();
@@ -1190,16 +1188,15 @@ TEST(BreakPointApiGetter) {
   // Run with breakpoint.
   bp = SetBreakPoint(function, 0);
   CompileRun("get_wrapper(o, 'f')");
-  CHECK_EQ(0, break_point_hit_count);
+  CHECK_EQ(1, break_point_hit_count);
 
   CompileRun("o.f");
-  CHECK_EQ(1, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("get_wrapper(o, 'f', 2)");
-  CompileRun("o.f");
-  CHECK_EQ(1, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();
@@ -1248,12 +1245,12 @@ TEST(BreakPointApiSetter) {
   CHECK_EQ(1, break_point_hit_count);
 
   CompileRun("set_wrapper(o, 'f', 2)");
-  CHECK_EQ(1, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("o.f = 3");
-  CHECK_EQ(1, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   // === Test API builtin as setter, with condition ===
   break_point_hit_count = 0;
@@ -1264,16 +1261,15 @@ TEST(BreakPointApiSetter) {
   CHECK_EQ(0, break_point_hit_count);
 
   CompileRun("set_wrapper(o, 'f', 3)");
-  CHECK_EQ(0, break_point_hit_count);
+  CHECK_EQ(1, break_point_hit_count);
 
   CompileRun("o.f = 3");
-  CHECK_EQ(1, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("set_wrapper(o, 'f', 2)");
-  CompileRun("o.f = 3");
-  CHECK_EQ(1, break_point_hit_count);
+  CHECK_EQ(2, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();