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(); + } +}