[debug] Forcibly instantiate lazy accessor pairs when setting break points.

Previously we had some kind of self-healing when calling lazy accessor
pairs via InvokeApiFunction(), but we also have other paths for calling
into FunctionTemplateInfos directly, which didn't do this check. Since
we already walk the heap when installing the DebugBreakTrampoline, and
compile all uncompiled functions, we can also just forcibly instantiate
all the lazy accessor pairs at that time and not have to worry about the
break-at-entry later.

Bug: v8:178, v8:7596, v8:8834
Cq-Include-Trybots: luci.chromium.try:linux-blink-rel
Change-Id: I514392cf328fc8ed0b80ad19009f32e20ff850b8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1565890Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60813}

src/api-natives.cc

@@ -69,15 +69,19 @@ MaybeHandle<Object> DefineAccessorProperty(
          !FunctionTemplateInfo::cast(*getter)->do_not_cache());
   DCHECK(!setter->IsFunctionTemplateInfo() ||
          !FunctionTemplateInfo::cast(*setter)->do_not_cache());
-  if (force_instantiate) {
   if (getter->IsFunctionTemplateInfo()) {
+    if (force_instantiate ||
+        FunctionTemplateInfo::cast(*getter)->BreakAtEntry()) {
       ASSIGN_RETURN_ON_EXCEPTION(
           isolate, getter,
           InstantiateFunction(isolate,
                               Handle<FunctionTemplateInfo>::cast(getter)),
           Object);
     }
+  }
   if (setter->IsFunctionTemplateInfo()) {
+    if (force_instantiate ||
+        FunctionTemplateInfo::cast(*setter)->BreakAtEntry()) {
       ASSIGN_RETURN_ON_EXCEPTION(
           isolate, setter,
           InstantiateFunction(isolate,
@@ -85,8 +89,9 @@ MaybeHandle<Object> DefineAccessorProperty(
           Object);
     }
   }
-  RETURN_ON_EXCEPTION(isolate, JSObject::DefineAccessor(object, name, getter,
-                                                        setter, attributes),
+  RETURN_ON_EXCEPTION(
+      isolate,
+      JSObject::DefineAccessor(object, name, getter, setter, attributes),
       Object);
   return object;
 }

src/builtins/builtins-api.cc

@@ -183,24 +183,10 @@ MaybeHandle<Object> Builtins::InvokeApiFunction(Isolate* isolate,
     }
   }
 
-  if (function->IsFunctionTemplateInfo()) {
-    Handle<FunctionTemplateInfo> info =
-        Handle<FunctionTemplateInfo>::cast(function);
-    // If we need to break at function entry, go the long way. Instantiate the
-    // function, use the DebugBreakTrampoline, and call it through JS.
-    if (info->BreakAtEntry()) {
-      DCHECK(!is_construct);
-      DCHECK(new_target->IsUndefined(isolate));
-      Handle<JSFunction> function;
-      ASSIGN_RETURN_ON_EXCEPTION(isolate, function,
-                                 ApiNatives::InstantiateFunction(
-                                     info, MaybeHandle<v8::internal::Name>()),
-                                 Object);
-      Handle<Code> trampoline = BUILTIN_CODE(isolate, DebugBreakTrampoline);
-      function->set_code(*trampoline);
-      return Execution::Call(isolate, function, receiver, argc, args);
-    }
-  }
+  // We assume that all lazy accessor pairs have been instantiated when setting
+  // a break point on any API function.
+  DCHECK_IMPLIES(function->IsFunctionTemplateInfo(),
+                 !Handle<FunctionTemplateInfo>::cast(function)->BreakAtEntry());
 
   Handle<FunctionTemplateInfo> fun_data =
       function->IsFunctionTemplateInfo()

src/debug/debug.cc

@@ -8,6 +8,7 @@
 #include <unordered_set>
 
 #include "src/api-inl.h"
+#include "src/api-natives.h"
 #include "src/arguments.h"
 #include "src/assembler-inl.h"
 #include "src/base/platform/mutex.h"
@@ -1225,6 +1226,7 @@ void Debug::InstallDebugBreakTrampoline() {
 
   Handle<Code> trampoline = BUILTIN_CODE(isolate_, DebugBreakTrampoline);
   std::vector<Handle<JSFunction>> needs_compile;
+  std::vector<Handle<AccessorPair>> needs_instantiate;
   {
     HeapIterator iterator(isolate_->heap());
     for (HeapObject obj = iterator.next(); !obj.is_null();
@@ -1242,9 +1244,37 @@ void Debug::InstallDebugBreakTrampoline() {
         } else {
           fun->set_code(*trampoline);
         }
+      } else if (obj->IsAccessorPair()) {
+        AccessorPair accessor_pair = AccessorPair::cast(obj);
+        if (accessor_pair->getter()->IsFunctionTemplateInfo() ||
+            accessor_pair->setter()->IsFunctionTemplateInfo()) {
+          needs_instantiate.push_back(handle(accessor_pair, isolate_));
         }
       }
     }
+  }
+
+  // Forcibly instantiate all lazy accessor pairs to make sure that they
+  // properly hit the debug break trampoline.
+  for (Handle<AccessorPair> accessor_pair : needs_instantiate) {
+    if (accessor_pair->getter()->IsFunctionTemplateInfo()) {
+      Handle<JSFunction> fun =
+          ApiNatives::InstantiateFunction(
+              handle(FunctionTemplateInfo::cast(accessor_pair->getter()),
+                     isolate_))
+              .ToHandleChecked();
+      accessor_pair->set_getter(*fun);
+    }
+    if (accessor_pair->setter()->IsFunctionTemplateInfo()) {
+      Handle<JSFunction> fun =
+          ApiNatives::InstantiateFunction(
+              handle(FunctionTemplateInfo::cast(accessor_pair->setter()),
+                     isolate_))
+              .ToHandleChecked();
+      accessor_pair->set_setter(*fun);
+    }
+  }
+
   // By overwriting the function code with DebugBreakTrampoline, which tailcalls
   // to shared code, we bypass CompileLazy. Perform CompileLazy here instead.
   for (Handle<JSFunction> fun : needs_compile) {

src/ic/ic.cc

@@ -870,8 +870,10 @@ Handle<Object> LoadIC::ComputeHandler(LookupIterator* lookup) {
           return slow_stub();
         }
 
-        if (getter->IsFunctionTemplateInfo() &&
-            FunctionTemplateInfo::cast(*getter)->BreakAtEntry()) {
+        if ((getter->IsFunctionTemplateInfo() &&
+             FunctionTemplateInfo::cast(*getter)->BreakAtEntry()) ||
+            (getter->IsJSFunction() &&
+             JSFunction::cast(*getter)->shared()->BreakAtEntry())) {
           // Do not install an IC if the api function has a breakpoint.
           TRACE_HANDLER_STATS(isolate(), LoadIC_SlowStub);
           return slow_stub();
@@ -1655,8 +1657,10 @@ MaybeObjectHandle StoreIC::ComputeHandler(LookupIterator* lookup) {
           return MaybeObjectHandle(slow_stub());
         }
 
-        if (setter->IsFunctionTemplateInfo() &&
-            FunctionTemplateInfo::cast(*setter)->BreakAtEntry()) {
+        if ((setter->IsFunctionTemplateInfo() &&
+             FunctionTemplateInfo::cast(*setter)->BreakAtEntry()) ||
+            (setter->IsJSFunction() &&
+             JSFunction::cast(*setter)->shared()->BreakAtEntry())) {
           // Do not install an IC if the api function has a breakpoint.
           TRACE_HANDLER_STATS(isolate(), StoreIC_SlowStub);
           return MaybeObjectHandle(slow_stub());

test/cctest/test-debug.cc

@@ -1167,11 +1167,27 @@ TEST(BreakPointApiAccessor) {
   CompileRun("set_loop();");
   CHECK_EQ(42, break_point_hit_count);
 
+  // Test that the break point also works when we install the function
+  // template on a new property (with a fresh AccessorPair instance).
+  v8::Local<v8::ObjectTemplate> baz_template =
+      v8::ObjectTemplate::New(env->GetIsolate());
+  baz_template->SetAccessorProperty(v8_str("g"), accessor_template,
+                                    accessor_template);
+  v8::Local<v8::Object> baz =
+      baz_template->NewInstance(env.local()).ToLocalChecked();
+  env->Global()->Set(env.local(), v8_str("b"), baz).ToChecked();
+
+  CompileRun("b.g = 4");
+  CHECK_EQ(43, break_point_hit_count);
+
+  CompileRun("b.g");
+  CHECK_EQ(44, break_point_hit_count);
+
   // Run without breakpoints.
   ClearBreakPoint(bp);
   CompileRun("o.f = 3");
   CompileRun("o.f");
-  CHECK_EQ(42, break_point_hit_count);
+  CHECK_EQ(44, break_point_hit_count);
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();