Introduce stack locals black list field on the ScopeInfo object

This CL is a prepatory step towards moving the stack locals blacklist
from the DebugEvaluateContext to the respective {ScopeInfo} objects.

The locals blacklist is used during local debug evaluate to
decide whether a context lookup can advance the context chain
upwards, or if lookup needs to stop at the current scope.

This CL also introduces a "Recreate" static helper method, that
allows an existing ScopeInfo to be cloned, but with a locals
blacklist attached. This will be needed since blacklists are only
created on-demand during debugging.

R=leszeks@chromium.org

Bug: chromium:1027475, v8:9938
Change-Id: I673dbc99ce9fdc84cb5cda3f9710ba2b76ab92ee
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1946349
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65380}

src/diagnostics/objects-printer.cc

@@ -2145,6 +2145,9 @@ void ScopeInfo::ScopeInfoPrint(std::ostream& os) {  // NOLINT
   if (HasOuterScopeInfo()) {
     os << "\n - outer scope info: " << Brief(OuterScopeInfo());
   }
+  if (HasLocalsBlackList()) {
+    os << "\n - locals blacklist: " << Brief(LocalsBlackList());
+  }
   if (HasFunctionName()) {
     os << "\n - function name: " << Brief(FunctionName());
   }

src/objects/scope-info.cc

@@ -209,7 +209,8 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
         PrivateNameLookupSkipsOuterClassField::encode(
             scope->private_name_lookup_skips_outer_class()) |
         HasContextExtensionSlotField::encode(scope->HasContextExtensionSlot()) |
-        IsReplModeScopeField::encode(scope->is_repl_mode_scope());
+        IsReplModeScopeField::encode(scope->is_repl_mode_scope()) |
+        HasLocalsBlackListField::encode(false);
     scope_info.SetFlags(flags);
 
     scope_info.SetParameterCount(parameter_count);
@@ -398,7 +399,8 @@ Handle<ScopeInfo> ScopeInfo::CreateForWithScope(
       ForceContextAllocationField::encode(false) |
       PrivateNameLookupSkipsOuterClassField::encode(false) |
       HasContextExtensionSlotField::encode(true) |
-      IsReplModeScopeField::encode(false);
+      IsReplModeScopeField::encode(false) |
+      HasLocalsBlackListField::encode(false);
   scope_info->SetFlags(flags);
 
   scope_info->SetParameterCount(0);
@@ -476,7 +478,8 @@ Handle<ScopeInfo> ScopeInfo::CreateForBootstrapping(Isolate* isolate,
               ForceContextAllocationField::encode(false) |
               PrivateNameLookupSkipsOuterClassField::encode(false) |
               HasContextExtensionSlotField::encode(is_native_context) |
-              IsReplModeScopeField::encode(false);
+              IsReplModeScopeField::encode(false) |
+              HasLocalsBlackListField::encode(false);
   scope_info->SetFlags(flags);
   scope_info->SetParameterCount(parameter_count);
   scope_info->SetContextLocalCount(context_local_count);
@@ -532,6 +535,39 @@ Handle<ScopeInfo> ScopeInfo::CreateForBootstrapping(Isolate* isolate,
   return scope_info;
 }
 
+// static
+Handle<ScopeInfo> ScopeInfo::RecreateWithBlackList(
+    Isolate* isolate, Handle<ScopeInfo> original, Handle<StringSet> blacklist) {
+  DCHECK(!original.is_null());
+  if (original->HasLocalsBlackList()) return original;
+
+  Handle<ScopeInfo> scope_info =
+      isolate->factory()->NewScopeInfo(original->length() + 1);
+
+  // Copy the static part first and update the flags to include the
+  // blacklist field, so {LocalsBlackListIndex} returns the correct value.
+  scope_info->CopyElements(isolate, 0, *original, 0, kVariablePartIndex,
+                           WriteBarrierMode::UPDATE_WRITE_BARRIER);
+  scope_info->SetFlags(
+      HasLocalsBlackListField::update(scope_info->Flags(), true));
+
+  // Copy the dynamic part including the provided blacklist:
+  //   1) copy all the fields up to the blacklist index
+  //   2) add the blacklist
+  //   3) copy the remaining fields
+  scope_info->CopyElements(
+      isolate, kVariablePartIndex, *original, kVariablePartIndex,
+      scope_info->LocalsBlackListIndex() - kVariablePartIndex,
+      WriteBarrierMode::UPDATE_WRITE_BARRIER);
+  scope_info->set(scope_info->LocalsBlackListIndex(), *blacklist);
+  scope_info->CopyElements(
+      isolate, scope_info->LocalsBlackListIndex() + 1, *original,
+      scope_info->LocalsBlackListIndex(),
+      scope_info->length() - scope_info->LocalsBlackListIndex() - 1,
+      WriteBarrierMode::UPDATE_WRITE_BARRIER);
+  return scope_info;
+}
+
 ScopeInfo ScopeInfo::Empty(Isolate* isolate) {
   return ReadOnlyRoots(isolate).empty_scope_info();
 }
@@ -678,6 +714,16 @@ bool ScopeInfo::IsReplModeScope() const {
   return IsReplModeScopeField::decode(Flags());
 }
 
+bool ScopeInfo::HasLocalsBlackList() const {
+  if (length() == 0) return false;
+  return HasLocalsBlackListField::decode(Flags());
+}
+
+StringSet ScopeInfo::LocalsBlackList() const {
+  DCHECK(HasLocalsBlackList());
+  return StringSet::cast(get(LocalsBlackListIndex()));
+}
+
 bool ScopeInfo::HasContext() const { return ContextLength() > 0; }
 
 Object ScopeInfo::FunctionName() const {
@@ -915,10 +961,14 @@ int ScopeInfo::OuterScopeInfoIndex() const {
   return PositionInfoIndex() + (HasPositionInfo() ? kPositionInfoEntries : 0);
 }
 
-int ScopeInfo::ModuleInfoIndex() const {
+int ScopeInfo::LocalsBlackListIndex() const {
   return OuterScopeInfoIndex() + (HasOuterScopeInfo() ? 1 : 0);
 }
 
+int ScopeInfo::ModuleInfoIndex() const {
+  return LocalsBlackListIndex() + (HasLocalsBlackList() ? 1 : 0);
+}
+
 int ScopeInfo::ModuleVariableCountIndex() const {
   return ModuleInfoIndex() + 1;
 }

src/objects/scope-info.h

@@ -10,6 +10,7 @@
 #include "src/objects/function-kind.h"
 #include "src/objects/objects.h"
 #include "src/utils/utils.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"  // nogncheck
 
 // Has to be the last include (doesn't have include guards):
 #include "src/objects/object-macros.h"
@@ -24,6 +25,7 @@ template <typename T>
 class MaybeHandle;
 class SourceTextModuleInfo;
 class Scope;
+class StringSet;
 class Zone;
 
 // ScopeInfo represents information about different scopes of a source
@@ -193,6 +195,14 @@ class ScopeInfo : public FixedArray {
   // Return the outer ScopeInfo if present.
   ScopeInfo OuterScopeInfo() const;
 
+  // Returns true if this ScopeInfo has a black list attached containing
+  // stack allocated local variables.
+  V8_EXPORT_PRIVATE bool HasLocalsBlackList() const;
+  // Returns a list of stack-allocated locals of parent scopes.
+  // Used during local debug-evalute to decide whether a context lookup
+  // can continue upwards after checking this scope.
+  V8_EXPORT_PRIVATE StringSet LocalsBlackList() const;
+
   // Returns true if this ScopeInfo was created for a scope that skips the
   // closest outer class when resolving private names.
   bool PrivateNameLookupSkipsOuterClass() const;
@@ -214,6 +224,13 @@ class ScopeInfo : public FixedArray {
   static Handle<ScopeInfo> CreateForNativeContext(Isolate* isolate);
   static Handle<ScopeInfo> CreateGlobalThisBinding(Isolate* isolate);
 
+  // Creates a copy of a {ScopeInfo} but with the provided locals blacklist
+  // attached. Does nothing if the original {ScopeInfo} already has a field
+  // for a blacklist reserved.
+  V8_EXPORT_PRIVATE static Handle<ScopeInfo> RecreateWithBlackList(
+      Isolate* isolate, Handle<ScopeInfo> original,
+      Handle<StringSet> blacklist);
+
   // Serializes empty scope info.
   V8_EXPORT_PRIVATE static ScopeInfo Empty(Isolate* isolate);
 
@@ -274,6 +291,7 @@ class ScopeInfo : public FixedArray {
   using HasContextExtensionSlotField =
       PrivateNameLookupSkipsOuterClassField::Next<bool, 1>;
   using IsReplModeScopeField = HasContextExtensionSlotField::Next<bool, 1>;
+  using HasLocalsBlackListField = IsReplModeScopeField::Next<bool, 1>;
 
   STATIC_ASSERT(kLastFunctionKind <= FunctionKindField::kMax);
 
@@ -309,10 +327,13 @@ class ScopeInfo : public FixedArray {
   //    the scope belongs to a function or script.
   // 8. OuterScopeInfoIndex:
   //    The outer scope's ScopeInfo or the hole if there's none.
-  // 9. SourceTextModuleInfo, ModuleVariableCount, and ModuleVariables:
-  //    For a module scope, this part contains the SourceTextModuleInfo, the
-  //    number of MODULE-allocated variables, and the metadata of those
-  //    variables.  For non-module scopes it is empty.
+  // 9. LocalsBlackList: List of stack allocated local variables. Used by
+  //    debug evaluate to properly abort variable lookup when a name clashes
+  //    with a stack allocated local that can't be materialized.
+  // 10. SourceTextModuleInfo, ModuleVariableCount, and ModuleVariables:
+  //     For a module scope, this part contains the SourceTextModuleInfo, the
+  //     number of MODULE-allocated variables, and the metadata of those
+  //     variables.  For non-module scopes it is empty.
   int ContextLocalNamesIndex() const;
   int ContextLocalInfosIndex() const;
   int SavedClassVariableInfoIndex() const;
@@ -321,6 +342,7 @@ class ScopeInfo : public FixedArray {
   int InferredFunctionNameIndex() const;
   int PositionInfoIndex() const;
   int OuterScopeInfoIndex() const;
+  V8_EXPORT_PRIVATE int LocalsBlackListIndex() const;
   int ModuleInfoIndex() const;
   int ModuleVariableCountIndex() const;
   int ModuleVariablesIndex() const;
@@ -358,6 +380,7 @@ class ScopeInfo : public FixedArray {
                                   ScopeInfo::VariableAllocationInfo var);
 
   OBJECT_CONSTRUCTORS(ScopeInfo, FixedArray);
+  FRIEND_TEST(TestWithNativeContext, RecreateScopeInfoWithLocalsBlacklistWorks);
 };
 
 std::ostream& operator<<(std::ostream& os,

src/objects/string-table.h

@@ -107,10 +107,11 @@ class StringSetShape : public BaseShape<String> {
 
 class StringSet : public HashTable<StringSet, StringSetShape> {
  public:
-  static Handle<StringSet> New(Isolate* isolate);
-  static Handle<StringSet> Add(Isolate* isolate, Handle<StringSet> blacklist,
-                               Handle<String> name);
-  bool Has(Isolate* isolate, Handle<String> name);
+  V8_EXPORT_PRIVATE static Handle<StringSet> New(Isolate* isolate);
+  V8_EXPORT_PRIVATE static Handle<StringSet> Add(Isolate* isolate,
+                                                 Handle<StringSet> blacklist,
+                                                 Handle<String> name);
+  V8_EXPORT_PRIVATE bool Has(Isolate* isolate, Handle<String> name);
 
   DECL_CAST(StringSet)
   OBJECT_CONSTRUCTORS(StringSet, HashTable<StringSet, StringSetShape>);

test/unittests/objects/object-unittest.cc

@@ -179,5 +179,43 @@ TEST_F(TestWithNativeContext, EmptyFunctionScopeInfo) {
             empty_function_scope_info->ContextLocalCount());
 }
 
+TEST_F(TestWithNativeContext, RecreateScopeInfoWithLocalsBlacklistWorks) {
+  // Create a JSFunction to get a {ScopeInfo} we can use for the test.
+  Handle<JSFunction> function = RunJS<JSFunction>("(function foo() {})");
+  Handle<ScopeInfo> original_scope_info(function->shared().scope_info(),
+                                        isolate());
+  ASSERT_FALSE(original_scope_info->HasLocalsBlackList());
+
+  Handle<String> foo_string =
+      isolate()->factory()->NewStringFromStaticChars("foo");
+  Handle<String> bar_string =
+      isolate()->factory()->NewStringFromStaticChars("bar");
+
+  Handle<StringSet> blacklist = StringSet::New(isolate());
+  StringSet::Add(isolate(), blacklist, foo_string);
+
+  Handle<ScopeInfo> scope_info = ScopeInfo::RecreateWithBlackList(
+      isolate(), original_scope_info, blacklist);
+
+  DisallowHeapAllocation no_gc;
+  EXPECT_TRUE(scope_info->HasLocalsBlackList());
+  EXPECT_TRUE(scope_info->LocalsBlackList().Has(isolate(), foo_string));
+  EXPECT_FALSE(scope_info->LocalsBlackList().Has(isolate(), bar_string));
+
+  EXPECT_EQ(original_scope_info->length() + 1, scope_info->length());
+
+  // Check that all variable fields *before* the blacklist stayed the same.
+  for (int i = ScopeInfo::kVariablePartIndex;
+       i < scope_info->LocalsBlackListIndex(); ++i) {
+    EXPECT_EQ(original_scope_info->get(i), scope_info->get(i));
+  }
+
+  // Check that all variable fields *after* the blacklist stayed the same.
+  for (int i = scope_info->LocalsBlackListIndex() + 1; i < scope_info->length();
+       ++i) {
+    EXPECT_EQ(original_scope_info->get(i - 1), scope_info->get(i));
+  }
+}
+
 }  // namespace internal
 }  // namespace v8