[vm] Variable scoping fixes

This CL fixes two bugs related to scoping: 1. Empty variable name lookup in outer functions failing. 2. Let expressions not pushing and popping contexts. Additionally, this CL introduces a `--print-scoping` debug flag to ease debugging scoping issues in the future. Variable lookup is now only using kernel offset, unless the offset is `kNoKernelOffset`. In that case, the names are used. All synthetic variables in the VM now use `kNoKernelOffset` as their offset. The let expressions not pushing contexts is fixed by pushing context. The context is only pushed if the scope captures variables. So this should not lead to performance regressions. (Any lacking pushing of scopes should have lead to crashes.) TEST=tests/ffi/regress_56412_2_test.dart TEST=tests/ffi/regress_56412_test.dart Closes: #56412 Change-Id: I85fc4a161b833df80ce8f2911d618aa2ac9797eb Cq-Include-Trybots: dart/try:vm-aot-android-release-arm64c-try,vm-aot-android-release-arm_x64-try,vm-aot-asan-linux-release-x64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-debug-x64c-try,vm-aot-mac-release-arm64-try,vm-aot-mac-release-x64-try,vm-aot-msan-linux-release-x64-try,vm-aot-obfuscate-linux-release-x64-try,vm-aot-optimization-level-linux-release-x64-try,vm-aot-tsan-linux-release-x64-try,vm-aot-ubsan-linux-release-x64-try,vm-aot-win-debug-arm64-try,vm-aot-win-debug-x64-try,vm-aot-win-debug-x64c-try,vm-appjit-linux-debug-x64-try,vm-asan-linux-release-arm64-try,vm-asan-linux-release-x64-try,vm-checked-mac-release-arm64-try,vm-eager-optimization-linux-release-ia32-try,vm-eager-optimization-linux-release-x64-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-ffi-qemu-linux-release-arm-try,vm-ffi-qemu-linux-release-riscv64-try,vm-fuchsia-release-arm64-try,vm-fuchsia-release-x64-try,vm-linux-debug-ia32-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-msan-linux-release-arm64-try,vm-msan-linux-release-x64-try,vm-reload-linux-debug-x64-try,vm-reload-rollback-linux-debug-x64-try,vm-tsan-linux-release-arm64-try,vm-tsan-linux-release-x64-try,vm-ubsan-linux-release-arm64-try,vm-ubsan-linux-release-x64-try,vm-win-debug-arm64-try,vm-win-debug-x64-try,vm-win-debug-x64c-try,vm-win-release-ia32-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/380983 Reviewed-by: Slava Egorov <[email protected]> Reviewed-by: Alexander Markov <[email protected]>
dart-lang · Aug 23, 2024 · 6378a26 · 6378a26
1 parent 87f7758
commit 6378a26
Show file tree

Hide file tree

Showing 11 changed files with 259 additions and 48 deletions.
diff --git a/BUILD.gn b/BUILD.gn
@@ -299,6 +299,8 @@ if (is_fuchsia) {
     "tests/ffi/regress_51538_test.dart",
     "tests/ffi/regress_52298_test.dart",
     "tests/ffi/regress_52399_test.dart",
+    "tests/ffi/regress_56412_2_test.dart",
+    "tests/ffi/regress_56412_test.dart",
     "tests/ffi/regress_b_261224444_test.dart",
     "tests/ffi/regress_flutter79441_test.dart",
     "tests/ffi/regress_flutter97301_test.dart",

diff --git a/runtime/vm/compiler/frontend/base_flow_graph_builder.h b/runtime/vm/compiler/frontend/base_flow_graph_builder.h
@@ -473,12 +473,6 @@ class BaseFlowGraphBuilder {
     return context_level_array_ != nullptr;
   }
 
-  // Sets current context level. It will be recorded for all subsequent
-  // deopt ids (until it is adjusted again).
-  void set_context_depth(intptr_t context_level) {
-    context_depth_ = context_level;
-  }
-
   // Reset context level for the given deopt id (which was allocated earlier).
   void reset_context_depth_for_deopt_id(intptr_t deopt_id);
 

diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
@@ -4280,10 +4280,15 @@ Fragment StreamingFlowGraphBuilder::BuildFunctionExpression() {
 }
 
 Fragment StreamingFlowGraphBuilder::BuildLet(TokenPosition* p) {
+  const intptr_t offset = ReaderOffset() - 1;  // Include the tag.
+
   const TokenPosition position = ReadPosition();  // read position.
   if (p != nullptr) *p = position;
-  Fragment instructions = BuildVariableDeclaration(nullptr);  // read variable.
-  instructions += BuildExpression();                          // read body.
+  Fragment instructions;
+  instructions += EnterScope(offset);
+  instructions += BuildVariableDeclaration(nullptr);  // read variable.
+  instructions += BuildExpression();                  // read body.
+  instructions += ExitScope(offset);
   return instructions;
 }
 

diff --git a/runtime/vm/compiler/frontend/scope_builder.cc b/runtime/vm/compiler/frontend/scope_builder.cc
@@ -4,9 +4,11 @@
 
 #include "vm/compiler/frontend/scope_builder.h"
 
+#include "vm/compiler/api/print_filter.h"
 #include "vm/compiler/backend/il.h"  // For CompileType.
 #include "vm/compiler/frontend/kernel_to_il.h"
 #include "vm/compiler/frontend/kernel_translation_helper.h"
+#include "vm/flags.h"
 
 namespace dart {
 namespace kernel {
@@ -25,7 +27,6 @@ ScopeBuilder::ScopeBuilder(ParsedFunction* parsed_function)
       current_function_scope_(nullptr),
       scope_(nullptr),
       depth_(0),
-      name_index_(0),
       needs_expr_temp_(false),
       helper_(
           zone_,
@@ -489,9 +490,45 @@ ScopeBuildingResult* ScopeBuilder::BuildScopes() {
                  (parsed_function_->suspend_state_var()->index().value() ==
                   SuspendState::kSuspendStateVarIndex));
 
+#if defined(DEBUG)
+  if (FLAG_print_scopes && compiler::PrintFilter::ShouldPrint(function)) {
+    THR_Print("===== Scopes for %s\n", function.ToFullyQualifiedCString());
+    THR_Print("%s", result_->ToCString());
+    THR_Print("=====\n");
+  }
+#endif
+
   return result_;
 }
 
+void ScopeBuildingResult::PrintTo(BaseTextBuffer* f) const {
+  f->AddString("== function scopes:\n");
+  for (int i = 0; i < function_scopes.length(); i++) {
+    auto scope = function_scopes[i];
+    scope.scope->PrintTo(f);
+  }
+
+  f->AddString("== all scopes, indexed by kernel_offset:\n");
+  auto it = scopes.GetIterator();
+  while (auto scope = it.Next()) {
+    f->Printf("%" Pd ": ", scope->key);
+    scope->value->PrintTo(f);
+  }
+
+  f->AddString("== all variables:\n");
+  auto it2 = locals.GetIterator();
+  while (auto local = it2.Next()) {
+    local->value->PrintTo(f);
+  }
+}
+
+const char* ScopeBuildingResult::ToCString() const {
+  char buffer[1024 * 16];
+  BufferFormatter f(buffer, sizeof(buffer));
+  PrintTo(&f);
+  return Thread::Current()->zone()->MakeCopyOfString(buffer);
+}
+
 void ScopeBuilder::ReportUnexpectedTag(const char* variant, Tag tag) {
   const auto& script = Script::Handle(Z, Script());
   H.ReportError(script, TokenPosition::kNoSource,
@@ -1344,9 +1381,7 @@ void ScopeBuilder::VisitVariableDeclaration() {
   helper.ReadUntilExcluding(VariableDeclarationHelper::kType);
   AbstractType& type = BuildAndVisitVariableType();
 
-  const String& name = (H.StringSize(helper.name_index_) == 0)
-                           ? GenerateName(":var", name_index_++)
-                           : H.DartSymbolObfuscate(helper.name_index_);
+  const String& name = H.DartSymbolObfuscate(helper.name_index_);
 
   intptr_t initializer_offset = helper_.ReaderOffset();
   Tag tag = helper_.ReadTag();  // read (first part of) initializer.
@@ -1881,11 +1916,8 @@ LocalVariable* ScopeBuilder::LookupVariable(
     // declared in an outer scope.  In that case, look it up in the scope by
     // name and add it to the variable map to simplify later lookup.
     ASSERT(current_function_scope_->parent() != nullptr);
-    StringIndex var_name = GetNameFromVariableDeclaration(
-        declaration_binary_offset - helper_.data_program_offset_,
-        parsed_function_->function());
 
-    const String& name = H.DartSymbolObfuscate(var_name);
+    const auto& name = Object::null_string();  // only use kernel offset.
     variable = current_function_scope_->parent()->LookupVariable(
         name, declaration_binary_offset, true);
     ASSERT(variable != nullptr);
@@ -1913,20 +1945,6 @@ LocalVariable* ScopeBuilder::LookupVariable(
   return variable;
 }
 
-StringIndex ScopeBuilder::GetNameFromVariableDeclaration(
-    intptr_t kernel_offset,
-    const Function& function) {
-  const auto& kernel_data = TypedDataView::Handle(Z, function.KernelLibrary());
-  ASSERT(!kernel_data.IsNull());
-
-  // Temporarily go to the variable declaration, read the name.
-  AlternativeReadingScopeWithNewData alt(&helper_.reader_, &kernel_data,
-                                         kernel_offset);
-  VariableDeclarationHelper helper(&helper_);
-  helper.ReadUntilIncluding(VariableDeclarationHelper::kNameIndex);
-  return helper.name_index_;
-}
-
 const String& ScopeBuilder::GenerateName(const char* prefix, intptr_t suffix) {
   char name[64];
   Utils::SNPrint(name, 64, "%s%" Pd "", prefix, suffix);

diff --git a/runtime/vm/compiler/frontend/scope_builder.h b/runtime/vm/compiler/frontend/scope_builder.h
@@ -128,9 +128,6 @@ class ScopeBuilder {
   // captured variable.
   LocalVariable* LookupVariable(intptr_t declaration_binary_offset);
 
-  StringIndex GetNameFromVariableDeclaration(intptr_t kernel_offset,
-                                             const Function& function);
-
   const String& GenerateName(const char* prefix, intptr_t suffix);
 
   void HandleLoadReceiver();
@@ -163,8 +160,6 @@ class ScopeBuilder {
   LocalScope* scope_;
   DepthState depth_;
 
-  intptr_t name_index_;
-
   bool needs_expr_temp_;
   TokenPosition first_body_token_position_ = TokenPosition::kNoSource;
 
@@ -233,6 +228,9 @@ class ScopeBuildingResult : public ZoneAllocated {
   // variables.
   GrowableArray<intptr_t> closure_offsets_without_captures;
 
+  void PrintTo(BaseTextBuffer* f) const;
+  const char* ToCString() const;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(ScopeBuildingResult);
 };

diff --git a/runtime/vm/flag_list.h b/runtime/vm/flag_list.h
@@ -160,6 +160,9 @@ constexpr bool FLAG_support_il_printer = false;
   P(polymorphic_with_deopt, bool, true,                                        \
     "Polymorphic calls with deoptimization / megamorphic call")                \
   P(precompiled_mode, bool, false, "Precompilation compiler mode")             \
+  D(print_scopes, bool, false,                                                 \
+    "Print scopes after scope building. Filtered by "                          \
+    "--print-flow-graph-filter.")                                              \
   P(print_snapshot_sizes, bool, false, "Print sizes of generated snapshots.")  \
   P(print_snapshot_sizes_verbose, bool, false,                                 \
     "Print cluster sizes of generated snapshots.")                             \

diff --git a/runtime/vm/parser.cc b/runtime/vm/parser.cc
@@ -198,10 +198,10 @@ void ParsedFunction::AllocateVariables() {
       tmp = Symbols::FromConcat(T, Symbols::OriginalParam(), variable->name());
 
       RELEASE_ASSERT(scope->LocalLookupVariable(
-                         tmp, variable->kernel_offset()) == nullptr);
+                         tmp, LocalVariable::kNoKernelOffset) == nullptr);
       raw_parameter = new LocalVariable(
           variable->declaration_token_pos(), variable->token_pos(), tmp,
-          variable->static_type(), variable->kernel_offset(),
+          variable->static_type(), LocalVariable::kNoKernelOffset,
           variable->inferred_type(), variable->inferred_arg_type(),
           variable->inferred_arg_value());
       raw_parameter->set_annotations_offset(variable->annotations_offset());
@@ -248,13 +248,13 @@ void ParsedFunction::AllocateVariables() {
       tmp = Symbols::FromConcat(T, Symbols::OriginalParam(),
                                 function_type_arguments_->name());
 
-      ASSERT(scope->LocalLookupVariable(
-                 tmp, function_type_arguments_->kernel_offset()) == nullptr);
+      ASSERT(scope->LocalLookupVariable(tmp, LocalVariable::kNoKernelOffset) ==
+             nullptr);
       raw_type_args_parameter =
           new LocalVariable(function_type_arguments_->declaration_token_pos(),
                             function_type_arguments_->token_pos(), tmp,
                             function_type_arguments_->static_type(),
-                            function_type_arguments_->kernel_offset());
+                            LocalVariable::kNoKernelOffset);
       bool ok = scope->AddVariable(raw_type_args_parameter);
       ASSERT(ok);
     }

diff --git a/runtime/vm/scopes.cc b/runtime/vm/scopes.cc
@@ -8,7 +8,6 @@
 #include "vm/compiler/backend/slot.h"
 #include "vm/kernel.h"
 #include "vm/object.h"
-#include "vm/object_store.h"
 #include "vm/stack_frame.h"
 #include "vm/symbols.h"
 
@@ -236,9 +235,12 @@ LocalVariable::LocalVariable(TokenPosition declaration_pos,
                         CompileType::kCanBeNull,
                         CompileType::kCannotBeSentinel))) {}
 
-// VM creates internal variables that start with ":"
+// The VM creates synthetic variables that start with ":" and the CFE creates
+// synthetic variables with no name.
 bool LocalVariable::IsFilteredIdentifier(const String& name) {
-  ASSERT(name.Length() > 0);
+  if (name.ptr() == Symbols::Empty().ptr()) {
+    return true;
+  }
   if (name.ptr() == Symbols::FunctionTypeArgumentsVar().ptr()) {
     // Keep :function_type_arguments for accessing type variables in debugging.
     return false;
@@ -335,13 +337,22 @@ void LocalScope::CollectLocalVariables(LocalVarDescriptorsBuilder* vars,
 
 LocalVariable* LocalScope::LocalLookupVariable(const String& name,
                                                intptr_t kernel_offset) const {
-  ASSERT(name.IsSymbol());
+  ASSERT(name.IsNull() || name.IsSymbol());
   for (intptr_t i = 0; i < variables_.length(); i++) {
     LocalVariable* var = variables_[i];
-    ASSERT(var->name().IsSymbol());
-    if ((var->name().ptr() == name.ptr()) &&
-        (var->kernel_offset() == kernel_offset)) {
-      return var;
+    if (var->kernel_offset() == kernel_offset) {
+      if (kernel_offset != LocalVariable::kNoKernelOffset) {
+        // Variable from kernel.
+        return var;
+      } else {
+        // Synthetic variable from the VM.
+        ASSERT(kernel_offset == LocalVariable::kNoKernelOffset);
+        ASSERT(name.IsSymbol());
+        ASSERT(var->name().IsSymbol());
+        if (var->name().ptr() == name.ptr()) {
+          return var;
+        }
+      }
     }
   }
   return nullptr;
@@ -578,6 +589,99 @@ ContextScopePtr LocalScope::CreateImplicitClosureScope(const Function& func) {
   return context_scope.ptr();
 }
 
+static void PrintIndentation(BaseTextBuffer* f, int depth) {
+  for (int i = 0; i < depth; i++) {
+    f->AddString("  ");
+  }
+}
+
+void LocalScope::PrintTo(BaseTextBuffer* f, int depth) const {
+  PrintIndentation(f, depth);
+  f->AddString("scope: ");
+  f->Printf("function_level: %i, ", function_level_);
+  f->Printf("loop_level: %i, ", loop_level_);
+  if (context_level_ == kUninitializedContextLevel) {
+    f->AddString("context_level: uninitialized, ");
+  } else {
+    f->Printf("context_level: %i, ", context_level_);
+  }
+  int num_variables_ = num_variables();
+  f->Printf("variables: %i, ", num_variables_);
+  f->Printf("context_variables: %i", num_context_variables());
+  if (function_level() == 1) {
+    f->Printf(", captured_variables: %i", NumCapturedVariables());
+  }
+  f->AddString("\n");
+
+  for (intptr_t i = 0; i < num_variables_; i++) {
+    LocalVariable* variable = VariableAt(i);
+    variable->PrintTo(f, "variable", depth + 1, this);
+  }
+
+  for (intptr_t i = 0; i < num_context_variables(); i++) {
+    LocalVariable* variable = context_variables().At(i);
+    variable->PrintTo(f, "context variable", depth + 1, this);
+  }
+
+  auto* child = child_;
+  while (child != nullptr) {
+    child->PrintTo(f, depth + 1);
+    child = child->sibling();
+  }
+}
+
+const char* LocalScope::ToCString() const {
+  char buffer[1024];
+  BufferFormatter f(buffer, sizeof(buffer));
+  PrintTo(&f);
+  return Thread::Current()->zone()->MakeCopyOfString(buffer);
+}
+
+void LocalVariable::PrintTo(BaseTextBuffer* f,
+                            const char* label,
+                            int depth,
+                            const LocalScope* scope) const {
+  PrintIndentation(f, depth);
+  f->Printf("%s: ", label);
+  const char* var_name = name().ToCString();
+  if (var_name[0] == '\0') {
+    // Follow CFE no-name variable printing convention.
+    var_name = "#t?";
+  }
+  f->Printf("%s, ", var_name);
+  f->Printf("kernel_offset: %" Pd ", ", kernel_offset());
+  if (HasIndex()) {
+    auto index_value = index().value();
+    if (index().value() == VariableIndex::kInvalidIndex) {
+      f->AddString("index: invalid");
+    } else {
+      f->Printf("index: %i", index_value);
+    }
+  } else {
+    f->AddString("index: none");
+  }
+  if (is_captured()) {
+    f->AddString(", is_captured");
+  }
+  if (is_captured_parameter()) {
+    f->AddString(", is_captured_parameter");
+  }
+  if (is_invisible()) {
+    f->AddString(", is_invisible");
+  }
+  if (scope != nullptr && owner() != scope) {
+    f->AddString(" (not owner)");
+  }
+  f->AddString("\n");
+}
+
+const char* LocalVariable::ToCString() const {
+  char buffer[1024];
+  BufferFormatter f(buffer, sizeof(buffer));
+  PrintTo(&f);
+  return Thread::Current()->zone()->MakeCopyOfString(buffer);
+}
+
 bool LocalVariable::ComputeIfIsAwaiterLink(const Library& library) {
   if (is_awaiter_link_ == IsAwaiterLink::kUnknown) {
     RELEASE_ASSERT(annotations_offset_ != kNoKernelOffset);

diff --git a/runtime/vm/scopes.h b/runtime/vm/scopes.h
@@ -217,6 +217,12 @@ class LocalVariable : public ZoneAllocated {
 
   bool Equals(const LocalVariable& other) const;
 
+  void PrintTo(BaseTextBuffer* f,
+               const char* label = "variable",
+               int depth = 0,
+               const LocalScope* scope = nullptr) const;
+  const char* ToCString() const;
+
  private:
   // If true, this variable is readonly.
   using IsFinalBit = BitField<uint32_t, bool, 0, 1>;
@@ -431,6 +437,9 @@ class LocalScope : public ZoneAllocated {
   // closure object.
   static ContextScopePtr CreateImplicitClosureScope(const Function& func);
 
+  void PrintTo(BaseTextBuffer* f, int depth = 0) const;
+  const char* ToCString() const;
+
  private:
   // Allocate the variable in the current context, possibly updating the current
   // context owner scope, if the variable is the first one to be allocated at