diff --git a/src/debug/debug-evaluate.cc b/src/debug/debug-evaluate.cc index 0d18bae5d4..2813456b12 100644 --- a/src/debug/debug-evaluate.cc +++ b/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 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 function_scope_info = handle( + frame_inspector_.GetFunction()->shared().scope_info(), isolate_); + Handle block_list = handle( + isolate_->LocalsBlockListCacheGet(function_scope_info), isolate_); + CHECK(block_list->IsStringSet()); + isolate_->LocalsBlockListCacheSet(scope_info, Handle::null(), + Handle::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); diff --git a/src/objects/contexts.cc b/src/objects/contexts.cc index 2a802b2d7f..e35e7ee7e4 100644 --- a/src/objects/contexts.cc +++ b/src/objects/contexts.cc @@ -203,6 +203,7 @@ Handle Context::Lookup(Handle context, Handle 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 Context::Lookup(Handle context, Handle name, reinterpret_cast(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 Context::Lookup(Handle context, Handle 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 Context::Lookup(Handle context, Handle 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 Context::Lookup(Handle context, Handle 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 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->previous(), isolate); } while (follow_context_chain); diff --git a/test/debugger/debug/debug-evaluate-shadowed-context-reuse.js b/test/debugger/debug/debug-evaluate-shadowed-context-reuse.js new file mode 100644 index 0000000000..afd74a520e --- /dev/null +++ b/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'); diff --git a/test/debugger/debug/debug-evaluate-shadowed-context.js b/test/debugger/debug/debug-evaluate-shadowed-context.js index 6d984fba1a..b91174b068 100644 --- a/test/debugger/debug/debug-evaluate-shadowed-context.js +++ b/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