From 6378a262120bee83da594da00fa26449ddfb10e1 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Fri, 23 Aug 2024 07:03:15 +0000 Subject: [PATCH] [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: https://github.com/dart-lang/sdk/issues/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 Reviewed-by: Alexander Markov --- BUILD.gn | 2 + .../frontend/base_flow_graph_builder.h | 6 - .../frontend/kernel_binary_flowgraph.cc | 9 +- runtime/vm/compiler/frontend/scope_builder.cc | 62 +++++---- runtime/vm/compiler/frontend/scope_builder.h | 8 +- runtime/vm/flag_list.h | 3 + runtime/vm/parser.cc | 10 +- runtime/vm/scopes.cc | 120 ++++++++++++++++-- runtime/vm/scopes.h | 9 ++ tests/ffi/regress_56412_2_test.dart | 45 +++++++ tests/ffi/regress_56412_test.dart | 33 +++++ 11 files changed, 259 insertions(+), 48 deletions(-) create mode 100644 tests/ffi/regress_56412_2_test.dart create mode 100644 tests/ffi/regress_56412_test.dart diff --git a/BUILD.gn b/BUILD.gn index 1d94b1dcf1f1..ddf90c351eb1 100644 --- 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 index e139b4c5a4c1..2272936ee389 100644 --- 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 index 6e6713ee9f22..0331259b000c 100644 --- 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 index fa54fde16ae0..2c336f421e1a 100644 --- 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 index 570e03432147..31d61c59949a 100644 --- 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 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 index 9f663e7cd3e8..aa06ebc98684 100644 --- 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 index 783cbc48a556..e1be656c05a5 100644 --- 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 index da03c6a6127f..af621df48531 100644 --- 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 index 59b3e382990f..437e5caf7dfb 100644 --- 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; @@ -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 diff --git a/tests/ffi/regress_56412_2_test.dart b/tests/ffi/regress_56412_2_test.dart new file mode 100644 index 000000000000..70c3d1f8e433 --- /dev/null +++ b/tests/ffi/regress_56412_2_test.dart @@ -0,0 +1,45 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Regression test for https://github.com/dart-lang/sdk/issues/56412. +// +// Exercises a variable in a parent function that has no name in kernel. +// After a function is compiled in the VM, unnamed variables are renamed to +// "var%i". But the lookup in the inner function still happens with "". + +import 'dart:ffi'; + +void main() async { + final myInstance = MyClass(); + try { + await myInstance.callMyMethod(); + } on Exception { + print('good'); + } +} + +final MyFinalizable myFinalizable = MyFinalizable(); + +class MyClass { + final int someVeryUniqueVariableName1337plus42 = 3; + + Object callMyMethod() { + return myFinalizable.myMethod( + namedArgument: Object(), + () { + // Force a capture of this. + // Without capture: `error: expected: delta >= 0`. + // With capture `error: expected: variable != nullptr`. + someVeryUniqueVariableName1337plus42; + throw Exception('Throw something'); + }, + ); + } +} + +class MyFinalizable implements Finalizable { + myMethod(Object Function() action, {Object? namedArgument}) { + return action(); + } +} diff --git a/tests/ffi/regress_56412_test.dart b/tests/ffi/regress_56412_test.dart new file mode 100644 index 000000000000..9a1906fa241e --- /dev/null +++ b/tests/ffi/regress_56412_test.dart @@ -0,0 +1,33 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Regression test for https://github.com/dart-lang/sdk/issues/56412. +// +// The combination of named arguments in any position and finalizables leads +// to nested let expressions. Let expressions introduce scopes. Often variables +// get hoisted into a parent scope which is usually a block. But this specific +// regression test has a variable declaration in a let scope. +// (The bug was that in the compiler, let expressions were not entering scopes.) + +import 'dart:ffi'; + +void main() { + MyFinalizable().myMethod( + namedArgument: Object(), + () { + final error = StateError('Cause crash'); + throw error; + }, + ); + print('done'); +} + +class MyFinalizable implements Finalizable { + Object myMethod( + Object Function() action, { + Object? namedArgument, + }) { + return Object(); + } +}