From afa22dd1abbce96f1e0fa0cf7c06293fc2a8b8a0 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Mon, 1 Mar 2021 13:13:19 +0000 Subject: [PATCH] [vm/aot] Disable assert strengthening optimization in AOT This optimization does not make sense in AOT and can actually cause AOT compiler to segfault because it expects environment to be attached to AssertAssignable instructions which is not always the case on AOT compiler. Fixes https://github.com/flutter/flutter/issues/76919 TEST=vm/cc/TypePropagator_RegressFlutter76919,vm/dart{,_2}/regress_flutter76919 Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-product-x64-try,vm-kernel-precomp-linux-release-x64-try Change-Id: I8f4314d7dac276833a9050bba835616b670a88d5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/188281 Reviewed-by: Martin Kustermann Commit-Queue: Vyacheslav Egorov --- .../vm/dart/regress_flutter76919_test.dart | 32 +++++++++++++ .../vm/dart_2/regress_flutter76919_test.dart | 32 +++++++++++++ .../vm/compiler/backend/type_propagator.cc | 9 +++- runtime/vm/compiler/backend/type_propagator.h | 1 + .../compiler/backend/type_propagator_test.cc | 46 +++++++++++++++++++ 5 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 runtime/tests/vm/dart/regress_flutter76919_test.dart create mode 100644 runtime/tests/vm/dart_2/regress_flutter76919_test.dart diff --git a/runtime/tests/vm/dart/regress_flutter76919_test.dart b/runtime/tests/vm/dart/regress_flutter76919_test.dart new file mode 100644 index 00000000000..669641a9346 --- /dev/null +++ b/runtime/tests/vm/dart/regress_flutter76919_test.dart @@ -0,0 +1,32 @@ +// Copyright (c) 2021, 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/flutter/flutter/issues/76919 +// Verifies that we don't try to strengthen an environmentless assertion +// with a class check in AOT mode which crashes AOT compiler. + +import 'dart:typed_data'; +import 'package:expect/expect.dart'; + +class C> { + final L list; + + C(this.list); + + @pragma('vm:never-inline') + E operator [](int index) => list[index]; + + @pragma('vm:never-inline') + void operator []=(int index, E value) { + // We emit AssertAssignable(value, E) on entry. + // Speculative compilation of this line produces CheckSmi(value) + list[index] = value; + } +} + +void main(List args) { + final v = C(Uint8List(1)); + v[0] = 1; + Expect.equals(1, v[0]); +} diff --git a/runtime/tests/vm/dart_2/regress_flutter76919_test.dart b/runtime/tests/vm/dart_2/regress_flutter76919_test.dart new file mode 100644 index 00000000000..669641a9346 --- /dev/null +++ b/runtime/tests/vm/dart_2/regress_flutter76919_test.dart @@ -0,0 +1,32 @@ +// Copyright (c) 2021, 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/flutter/flutter/issues/76919 +// Verifies that we don't try to strengthen an environmentless assertion +// with a class check in AOT mode which crashes AOT compiler. + +import 'dart:typed_data'; +import 'package:expect/expect.dart'; + +class C> { + final L list; + + C(this.list); + + @pragma('vm:never-inline') + E operator [](int index) => list[index]; + + @pragma('vm:never-inline') + void operator []=(int index, E value) { + // We emit AssertAssignable(value, E) on entry. + // Speculative compilation of this line produces CheckSmi(value) + list[index] = value; + } +} + +void main(List args) { + final v = C(Uint8List(1)); + v[0] = 1; + Expect.equals(1, v[0]); +} diff --git a/runtime/vm/compiler/backend/type_propagator.cc b/runtime/vm/compiler/backend/type_propagator.cc index fd01d698402..dce6e906514 100644 --- a/runtime/vm/compiler/backend/type_propagator.cc +++ b/runtime/vm/compiler/backend/type_propagator.cc @@ -48,6 +48,7 @@ void FlowGraphTypePropagator::Propagate(FlowGraph* flow_graph) { FlowGraphTypePropagator::FlowGraphTypePropagator(FlowGraph* flow_graph) : FlowGraphVisitor(flow_graph->reverse_postorder()), flow_graph_(flow_graph), + is_aot_(CompilerState::Current().is_aot()), visited_blocks_(new (flow_graph->zone()) BitVector(flow_graph->zone(), flow_graph->reverse_postorder().length())), @@ -119,7 +120,13 @@ void FlowGraphTypePropagator::PropagateRecursive(BlockEntryInstr* block) { const intptr_t rollback_point = rollback_.length(); - StrengthenAsserts(block); + if (!is_aot_) { + // Don't try to strengthen asserts with class checks in AOT mode, this is a + // speculative optimization which only really makes sense in JIT mode. + // It is also written to expect environments to be attached to + // AssertAssignable instructions, which is not always a case in AOT mode. + StrengthenAsserts(block); + } block->Accept(this); diff --git a/runtime/vm/compiler/backend/type_propagator.h b/runtime/vm/compiler/backend/type_propagator.h index 5c274206c20..435ef4a4f1c 100644 --- a/runtime/vm/compiler/backend/type_propagator.h +++ b/runtime/vm/compiler/backend/type_propagator.h @@ -77,6 +77,7 @@ class FlowGraphTypePropagator : public FlowGraphVisitor { Zone* zone() const { return flow_graph_->zone(); } FlowGraph* flow_graph_; + const bool is_aot_; BitVector* visited_blocks_; diff --git a/runtime/vm/compiler/backend/type_propagator_test.cc b/runtime/vm/compiler/backend/type_propagator_test.cc index 1090bc9cd90..05620b4ec36 100644 --- a/runtime/vm/compiler/backend/type_propagator_test.cc +++ b/runtime/vm/compiler/backend/type_propagator_test.cc @@ -491,4 +491,50 @@ class Cset_type_check_mode(LocalVariable::kTypeCheckedByCaller); + H.flow_graph()->parsed_function().scope()->AddVariable(v0_var); + + auto normal_entry = H.flow_graph()->graph_entry()->normal_entry(); + + // We are going to build the following graph: + // + // B0[graph_entry]: + // B1[function_entry]: + // v0 <- Parameter(0) + // AssertAssignable(v0, 'int') + // CheckSmi(v0) + // Return(v0) + + { + BlockBuilder builder(H.flow_graph(), normal_entry); + Definition* v0 = builder.AddParameter(0, 0, /*with_frame=*/true, kTagged); + auto null_value = builder.AddNullDefinition(); + builder.AddDefinition(new AssertAssignableInstr( + InstructionSource(), new Value(v0), + new Value( + H.flow_graph()->GetConstant(Type::ZoneHandle(Type::IntType()))), + new Value(null_value), new Value(null_value), Symbols::Value(), + S.GetNextDeoptId())); + builder.AddInstruction(new CheckSmiInstr(new Value(v0), S.GetNextDeoptId(), + InstructionSource())); + builder.AddReturn(new Value(v0)); + } + + H.FinishGraph(); + + H.flow_graph()->EliminateEnvironments(); + FlowGraphTypePropagator::Propagate(H.flow_graph()); +} + } // namespace dart