[debug] Re-use block lists across multiple local debug-evaluates

This CL implements the heavy lifting for re-using block lists:

  - On local debug-evaluate, we check if the paused function already
    has a block list. If not, we do a full re-parse, calculate the
    block lists and stash them in the global map.

  - On a context lookup, we do the lookup slightly differently. The
    block lists now store "outer" locals, so we need to check the
    block list before we advance to the next context, not before we
    do the lookup in the current context.

The CL also duplicates the debugger test that checks most of these
shadowing edge cases. While we keep working on the new feature
we still want to check both configurations, but the feature is too
small to warrant a separate bot. Note that the file with the flag
enabled has one additional test case that fails with the old
implementation. Unfortunately it's non-trivial to fix in the old
implementation.

This CL drastically improves performance for conditional breakpoints
as they use local debug-evaluate under the hood. The worst case
example (https://crbug.com/1072939#c15) improves from 6.5 seconds
to 100ms.

R=jarin@chromium.org

Bug: chromium:1363561
Change-Id: I85f3d908d246f0d2e31ed272f4db6a852b9dbc39
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3941584
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83665}

src/debug/debug-evaluate.cc

@@ -19,6 +19,7 @@
 #include "src/interpreter/bytecodes.h"
 #include "src/objects/code-inl.h"
 #include "src/objects/contexts.h"
+#include "src/objects/string-set-inl.h"
 
 #if V8_ENABLE_WEBASSEMBLY
 #include "src/debug/debug-wasm-objects.h"
@@ -210,7 +211,9 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
     : isolate_(isolate),
       frame_inspector_(frame, inlined_jsframe_index, isolate),
       scope_iterator_(isolate, &frame_inspector_,
-                      ScopeIterator::ReparseStrategy::kScript) {
+                      v8_flags.experimental_reuse_locals_blocklists
+                          ? ScopeIterator::ReparseStrategy::kScriptIfNeeded
+                          : ScopeIterator::ReparseStrategy::kScript) {
   Handle<Context> outer_context(frame_inspector_.GetFunction()->context(),
                                 isolate);
   evaluation_context_ = outer_context;
@@ -246,9 +249,16 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
     if (scope_iterator_.HasContext()) {
       context_chain_element.wrapped_context = scope_iterator_.CurrentContext();
     }
-    if (!scope_iterator_.InInnerScope() &&
-        !v8_flags.experimental_reuse_locals_blocklists) {
-      context_chain_element.blocklist = scope_iterator_.GetLocals();
+    if (v8_flags.experimental_reuse_locals_blocklists) {
+      // With the re-use experiment we only need `DebugEvaluateContexts` up
+      // to (and including) the paused function scope so the evaluated
+      // expression can access the materialized stack locals.
+      if (!scope_iterator_.InInnerScope()) break;
+    } else {
+      CHECK(!v8_flags.experimental_reuse_locals_blocklists);
+      if (!scope_iterator_.InInnerScope()) {
+        context_chain_element.blocklist = scope_iterator_.GetLocals();
+      }
     }
     context_chain_.push_back(context_chain_element);
   }
@@ -262,10 +272,28 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
     ContextChainElement element = *rit;
     scope_info = ScopeInfo::CreateForWithScope(isolate, scope_info);
     scope_info->SetIsDebugEvaluateScope();
-    if (!element.blocklist.is_null()) {
+
+    if (v8_flags.experimental_reuse_locals_blocklists) {
+      if (rit == context_chain_.rbegin()) {
+        // The DebugEvaluateContext we create for the closure scope is the only
+        // DebugEvaluateContext with a block list. This means we'll retrieve
+        // the existing block list from the paused function scope
+        // and also associate the temporary scope_info we create here with that
+        // blocklist.
+        Handle<ScopeInfo> function_scope_info = handle(
+            frame_inspector_.GetFunction()->shared().scope_info(), isolate_);
+        Handle<Object> block_list = handle(
+            isolate_->LocalsBlockListCacheGet(function_scope_info), isolate_);
+        CHECK(block_list->IsStringSet());
+        isolate_->LocalsBlockListCacheSet(scope_info, Handle<ScopeInfo>::null(),
+                                          Handle<StringSet>::cast(block_list));
+      }
+    } else if (!element.blocklist.is_null()) {
+      CHECK(!v8_flags.experimental_reuse_locals_blocklists);
       scope_info = ScopeInfo::RecreateWithBlockList(isolate, scope_info,
                                                     element.blocklist);
     }
+
     evaluation_context_ = factory->NewDebugEvaluateContext(
         evaluation_context_, scope_info, element.materialized_object,
         element.wrapped_context);

src/objects/contexts.cc

@@ -203,6 +203,7 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
   Isolate* isolate = context->GetIsolate();
 
   bool follow_context_chain = (flags & FOLLOW_CONTEXT_CHAIN) != 0;
+  bool has_seen_debug_evaluate_context = false;
   *index = kNotFound;
   *attributes = ABSENT;
   *init_flag = kCreatedInitialized;
@@ -223,6 +224,7 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
              reinterpret_cast<void*>(context->ptr()));
       if (context->IsScriptContext()) PrintF(" (script context)");
       if (context->IsNativeContext()) PrintF(" (native context)");
+      if (context->IsDebugEvaluateContext()) PrintF(" (debug context)");
       PrintF("\n");
     }
 
@@ -381,6 +383,8 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
         }
       }
     } else if (context->IsDebugEvaluateContext()) {
+      has_seen_debug_evaluate_context = true;
+
       // Check materialized locals.
       Object ext = context->get(EXTENSION_INDEX);
       if (ext.IsJSReceiver()) {
@@ -395,6 +399,8 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
 
       // Check blocklist. Names that are listed, cannot be resolved further.
       ScopeInfo scope_info = context->scope_info();
+      CHECK_IMPLIES(v8_flags.experimental_reuse_locals_blocklists,
+                    !scope_info.HasLocalsBlockList());
       if (scope_info.HasLocalsBlockList() &&
           scope_info.LocalsBlockList().Has(isolate, name)) {
         if (v8_flags.trace_contexts) {
@@ -417,6 +423,27 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
     // 3. Prepare to continue with the previous (next outermost) context.
     if (context->IsNativeContext()) break;
 
+    // In case we saw any DebugEvaluateContext, we'll need to check the block
+    // list before we can advance to properly "shadow" stack-allocated
+    // variables.
+    // Note that this implicitly skips the block list check for the
+    // "wrapped" context lookup for DebugEvaluateContexts. In that case
+    // `has_seen_debug_evaluate_context` will always be false.
+    if (v8_flags.experimental_reuse_locals_blocklists &&
+        has_seen_debug_evaluate_context &&
+        isolate->heap()->locals_block_list_cache().IsEphemeronHashTable()) {
+      Handle<ScopeInfo> scope_info = handle(context->scope_info(), isolate);
+      Object maybe_outer_block_list =
+          isolate->LocalsBlockListCacheGet(scope_info);
+      if (maybe_outer_block_list.IsStringSet() &&
+          StringSet::cast(maybe_outer_block_list).Has(isolate, name)) {
+        if (v8_flags.trace_contexts) {
+          PrintF(" - name is blocklisted. Aborting.\n");
+        }
+        break;
+      }
+    }
+
     context = Handle<Context>(context->previous(), isolate);
   } while (follow_context_chain);
 

test/debugger/debug/debug-evaluate-shadowed-context-reuse.js

@@ -0,0 +1,248 @@
+// Copyright 2015 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: --no-analyze-environment-liveness --experimental-reuse-locals-blocklists
+
+// Test that debug-evaluate only resolves variables that are used by
+// the function inside which we debug-evaluate. This is to avoid
+// incorrect variable resolution when a context-allocated variable is
+// shadowed by a stack-allocated variable.
+//
+// This test is an exact copy of `debug-evaluate-shadowed-context` modulo an
+// experimental flag. While the feature is in developement, we want to test both
+// configurations without having to introduce a separate bot.
+
+Debug = debug.Debug
+
+let test_name;
+let listener_delegate;
+let listener_called;
+let exception;
+let begin_test_count = 0;
+let end_test_count = 0;
+let break_count = 0;
+
+// Debug event listener which delegates.
+function listener(event, exec_state, event_data, data) {
+  try {
+    if (event == Debug.DebugEvent.Break) {
+      break_count++;
+      listener_called = true;
+      listener_delegate(exec_state);
+    }
+  } catch (e) {
+    exception = e;
+    print(e, e.stack);
+  }
+}
+Debug.setListener(listener);
+
+function BeginTest(name) {
+  test_name = name;
+  listener_called = false;
+  exception = null;
+  begin_test_count++;
+}
+
+function EndTest() {
+  assertTrue(listener_called, "listener not called for " + test_name);
+  assertNull(exception, test_name + " / " + exception);
+  end_test_count++;
+}
+
+BeginTest("Check that 'x' resolves correctly and 'a' is written correctly");
+var a = "a";
+function f1() {
+  var x = 1;     // context allocate x
+  (() => x);
+  var y = "y";
+  var z = "z";
+  (function () {
+    var x = 2;   // stack allocate shadowing x
+    (function () {
+      y;         // access y
+      debugger;  // ReferenceError
+    })();        // 2
+  })();          // 1
+  return y;
+}
+
+listener_delegate = function(exec_state) {
+  for (var i = 0; i < exec_state.frameCount() - 1; i++) {
+    var frame = exec_state.frame(i);
+    var value;
+    try {
+      value = frame.evaluate("x").value();
+    } catch (e) {
+      value = e.name;
+    }
+    print(frame.sourceLineText());
+    var expected = frame.sourceLineText().match(/\/\/ (.*$)/)[1];
+    assertEquals(String(expected), String(value));
+  }
+  assertEquals("[object global]",
+                String(exec_state.frame(0).evaluate("this").value()));
+  assertEquals("y", exec_state.frame(0).evaluate("y").value());
+  assertEquals("a", exec_state.frame(0).evaluate("a").value());
+  exec_state.frame(0).evaluate("a = 'A'");
+  assertThrows(() => exec_state.frame(0).evaluate("z"), ReferenceError);
+}
+f1();
+assertEquals("A", a);
+a = "a";
+EndTest();
+
+BeginTest("Check that a context-allocated 'this' works")
+function f2() {
+  var x = 1;     // context allocate x
+  (() => x);
+  var y = "y";
+  var z = "z";
+  (function() {
+    var x = 2;   // stack allocate shadowing x
+    (() => {
+      y;
+      a;
+      this;      // context allocate receiver
+      debugger;  // ReferenceError
+    })();        // 2
+  })();          // 1
+  return y;
+};
+
+// Uses the same listener delgate as for `f1`.
+f2();
+assertEquals("A", a);
+EndTest();
+
+BeginTest("Check that we don't get confused with nested scopes");
+function f3() {
+  var x = 1;     // context allocate x
+  (() => x);
+  (function() {
+    var x = 2;   // stack allocate shadowing x
+    (function() {
+      {          // context allocate x in a nested scope
+        let x = 3;
+        (() => x);
+      }
+      debugger;
+    })();
+  })();
+}
+
+listener_delegate = function(exec_state) {
+  assertThrows(() => exec_state.frame(0).evaluate("x").value());
+}
+f3();
+EndTest();
+
+BeginTest("Check that stack-allocated variable is unavailable");
+function f4() {
+  let a = 1;
+  let b = 2;
+  let c = 3;
+  () => a + c;  // a and c are context-allocated
+  return function g() {
+    let a = 2;  // a is stack-allocated
+    return function h() {
+      b;  // b is allocated onto f's context.
+      debugger;
+    }
+  }
+}
+
+listener_delegate = function(exec_state) {
+  assertEquals(2, exec_state.frame(0).evaluate("b").value());
+  assertEquals(3, exec_state.frame(0).evaluate("c").value())
+  assertThrows(() => exec_state.frame(0).evaluate("a").value());
+};
+(f4())()();
+EndTest();
+
+BeginTest("Check that block lists on the closure boundary work as expected");
+function f5() {
+  let a = 1;
+  () => a;  // a is context-allocated
+  return function g() {
+    let a = 2;  // a is stack-allocated
+    {
+      let b = 3;
+      return function h() {
+        debugger;
+      }
+    }
+  }
+}
+
+listener_delegate = function(exec_state) {
+  assertThrows(() => exec_state.frame(0).evaluate("a").value());
+};
+(f5())()();
+EndTest();
+
+BeginTest("Check that outer functions also get the correct block list calculated");
+// This test is important once we re-use block list info. The block list for `g`
+// needs to be correctly calculated already when we stop on break_position 1.
+
+let break_position;
+function f6() {
+  let a = 1;                             // stack-allocated
+  return function g() {                  // g itself doesn't require a context.
+    if (break_position === 2) debugger;
+    let a = 2; (() => a);                // context-allocated
+    return function h() {
+      if (break_position === 1) debugger;
+    }
+  }
+}
+
+listener_delegate = function (exec_state) {
+  assertEquals(2, exec_state.frame(0).evaluate("a").value());
+}
+break_position = 1;
+(f6())()();
+EndTest();
+
+BeginTest("Check that outer functions also get the correct block list calculated (continued)");
+listener_delegate = function (exec_state) {
+  assertThrows(() => exec_state.frame(0).evaluate("a").value());
+}
+break_position = 2;
+(f6())()();
+EndTest();
+
+BeginTest("Check that 'inner' block lists, calculated on a previous pause, don't block the lookup");
+
+function f7(o) {
+  let a = 1;  // stack-allocated.
+  with (o) {  // create a with-scope whos block-list has 'a' in it.
+    if (break_position === 2) debugger;
+    (function g() {
+      if (break_position === 1) debugger; // Trigger block-list calculation for the with-scope.
+    })();
+  }
+}
+
+listener_delegate = function (exec_state) {
+  assertThrows(() => exec_state.frame(0).evaluate("a").value());
+}
+break_position = 1;
+f7({});
+EndTest();
+
+BeginTest("Check that 'inner' block lists, calculated on a previous pause, don't block the lookup (continued)");
+// The second time we pause the with-scope already has a block-list, but 'a' should be accessible as a
+// materialized stack-local.
+listener_delegate = function (exec_state) {
+  assertEquals(1, exec_state.frame(0).evaluate("a").value());
+}
+break_position = 2;
+f7({});
+EndTest();
+
+assertEquals(begin_test_count, break_count,
+  'one or more tests did not enter the debugger');
+assertEquals(begin_test_count, end_test_count,
+  'one or more tests did not have its result checked');

test/debugger/debug/debug-evaluate-shadowed-context.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --no-analyze-environment-liveness
+// Flags: --no-analyze-environment-liveness --no-experimental-reuse-locals-blocklists
 
 // Test that debug-evaluate only resolves variables that are used by
 // the function inside which we debug-evaluate. This is to avoid