dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

[vm] Variable scoping fixes

Browse Source 
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 <vegorov@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
This commit is contained in:
Daco Harkes
2024-08-23 07:03:15 +00:00
parent 87f7758b88
commit 6378a26212
11 changed files with 259 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
BUILD.gn
+2
View File
@@ -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",
runtime/vm/compiler/frontend/base_flow_graph_builder.h
-6
View File
@@ -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);
runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
+7 -2
View File
@@ -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;
}
runtime/vm/compiler/frontend/scope_builder.cc
+40 -22
View File
@@ -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);
runtime/vm/compiler/frontend/scope_builder.h
+3 -5
View File
@@ -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);
};
runtime/vm/flag_list.h
+3
View File
@@ -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.") \
runtime/vm/parser.cc
+5 -5
View File
@@ -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);
}
runtime/vm/scopes.cc
+112 -8
View File
@@ -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);
runtime/vm/scopes.h
+9
View File
@@ -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
tests/ffi/regress_56412_2_test.dart
+45
View File
@@ -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();
}
}
tests/ffi/regress_56412_test.dart
+33
View File
@@ -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();
}
}