[vm/compiler] Fix assertion when assigning type to a Phi in ComputeSSA
Fixes https://github.com/dart-lang/sdk/issues/43682
Change-Id: I6b123ac5ba153b1f4177148341619419c3058684
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166304
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
diff --git a/pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart b/pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart
index af42f14..2ae32bf 100644
--- a/pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart
+++ b/pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart
@@ -377,9 +377,7 @@
factMap[variable],
isCheck.functionNestingLevel,
isCheck.checkedType,
- []
- ..addAll(isCheck._blockingScopes)
- ..add(_currentScope));
+ [...isCheck._blockingScopes, _currentScope]);
factMap[variable] = facts;
_factCacheState = facts;
}
diff --git a/runtime/tests/vm/dart/regress_43682_test.dart b/runtime/tests/vm/dart/regress_43682_test.dart
new file mode 100644
index 0000000..63ef705
--- /dev/null
+++ b/runtime/tests/vm/dart/regress_43682_test.dart
@@ -0,0 +1,32 @@
+// Copyright (c) 2020, 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.
+
+// VMOptions=--deterministic --optimization_counter_threshold=20
+
+// Verifies that SSA construction doesn't crash when handling a Phi
+// corresponding to an expression temp in case of OSR with non-empty
+// expression stack.
+// Regression test for https://github.com/dart-lang/sdk/issues/43682.
+
+import 'package:expect/expect.dart';
+
+class Foo {
+ List<Object> data;
+ Foo(this.data);
+}
+
+Map<String, Foo> foo(List<Object> objects) {
+ Map<String, Foo> map = {};
+ // OSR happens during '...objects' spread, and Foo instance is already
+ // allocated and remains on the stack during OSR.
+ // OSR Phi corresponding to that value is stored into 'foo' local and
+ // then loaded from it, but it doesn't correspond to 'foo' environment slot.
+ final foo = new Foo([...objects]);
+ map['hi'] = foo;
+ return map;
+}
+
+main() {
+ Expect.equals(30, foo(List.filled(30, Object()))['hi']!.data.length);
+}
diff --git a/runtime/tests/vm/dart_2/regress_43682_test.dart b/runtime/tests/vm/dart_2/regress_43682_test.dart
new file mode 100644
index 0000000..15924b1
--- /dev/null
+++ b/runtime/tests/vm/dart_2/regress_43682_test.dart
@@ -0,0 +1,32 @@
+// Copyright (c) 2020, 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.
+
+// VMOptions=--deterministic --optimization_counter_threshold=20
+
+// Verifies that SSA construction doesn't crash when handling a Phi
+// corresponding to an expression temp in case of OSR with non-empty
+// expression stack.
+// Regression test for https://github.com/dart-lang/sdk/issues/43682.
+
+import 'package:expect/expect.dart';
+
+class Foo {
+ List<Object> data;
+ Foo(this.data);
+}
+
+Map<String, Foo> foo(List<Object> objects) {
+ Map<String, Foo> map = {};
+ // OSR happens during '...objects' spread, and Foo instance is already
+ // allocated and remains on the stack during OSR.
+ // OSR Phi corresponding to that value is stored into 'foo' local and
+ // then loaded from it, but it doesn't correspond to 'foo' environment slot.
+ final foo = new Foo([...objects]);
+ map['hi'] = foo;
+ return map;
+}
+
+main() {
+ Expect.equals(30, foo(List.filled(30, Object()))['hi'].data.length);
+}
diff --git a/runtime/vm/compiler/backend/flow_graph.cc b/runtime/vm/compiler/backend/flow_graph.cc
index 9196b2d..82430b4 100644
--- a/runtime/vm/compiler/backend/flow_graph.cc
+++ b/runtime/vm/compiler/backend/flow_graph.cc
@@ -1453,13 +1453,23 @@
// there as incoming value by renaming or it was stored there by
// StoreLocal which took this Phi from another local via LoadLocal,
// to which this reasoning applies recursively.
+ //
// This means that we are guaranteed to process LoadLocal for a
- // matching variable first.
+ // matching variable first, unless there was an OSR with a non-empty
+ // expression stack. In the latter case, Phi inserted by
+ // FlowGraph::AddSyntheticPhis for expression temp will not have an
+ // assigned type and may be accessed by StoreLocal and subsequent
+ // LoadLocal.
+ //
if (!phi->HasType()) {
- ASSERT((index < phi->block()->phis()->length()) &&
- ((*phi->block()->phis())[index] == phi));
- phi->UpdateType(
- CompileType::FromAbstractType(load->local().type()));
+ // Check if phi corresponds to the same slot.
+ auto* phis = phi->block()->phis();
+ if ((index < phis->length()) && (*phis)[index] == phi) {
+ phi->UpdateType(
+ CompileType::FromAbstractType(load->local().type()));
+ } else {
+ ASSERT(IsCompiledForOsr() && (phi->block()->stack_depth() > 0));
+ }
}
}
break;
