From 68fd2a9d52802bdcc2db31d9b31a2e128b392f30 Mon Sep 17 00:00:00 2001 From: Aske Simon Christensen Date: Fri, 2 Oct 2020 16:17:58 +0000 Subject: [PATCH] [vm/jit] Pessimize type assumption in LoadLocal of covariant parameter. This avoids unsound optimizations that could arise from loading a parameter before its type had been checked. Affects only unoptimized code. Fixes https://github.com/dart-lang/sdk/issues/43464 Some considerations for a cleaner fix are described in https://github.com/dart-lang/sdk/issues/43654 Change-Id: I05872e46495313e82e9c516e5f283e1bc4612300 Cq-Do-Not-Cancel-Tryjobs: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/164500 Commit-Queue: Aske Simon Christensen Reviewed-by: Martin Kustermann --- runtime/tests/vm/dart/regress_43464_test.dart | 21 +++++++++++++++++ .../tests/vm/dart_2/regress_43464_test.dart | 21 +++++++++++++++++ .../vm/compiler/backend/type_propagator.cc | 6 +++++ runtime/vm/compiler/frontend/scope_builder.cc | 3 +++ runtime/vm/parser.cc | 3 +++ runtime/vm/scopes.h | 23 +++++++++++++++---- 6 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 runtime/tests/vm/dart/regress_43464_test.dart create mode 100644 runtime/tests/vm/dart_2/regress_43464_test.dart diff --git a/runtime/tests/vm/dart/regress_43464_test.dart b/runtime/tests/vm/dart/regress_43464_test.dart new file mode 100644 index 00000000000..43a3c1135bd --- /dev/null +++ b/runtime/tests/vm/dart/regress_43464_test.dart @@ -0,0 +1,21 @@ +// 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. + +import 'package:expect/expect.dart'; + +class A {} + +abstract class B { + dynamic foo(T a); +} + +class C extends B { + dynamic foo(A a) { + return () => a; + } +} + +main() { + Expect.throws(() => (C().foo as dynamic)(1)); +} diff --git a/runtime/tests/vm/dart_2/regress_43464_test.dart b/runtime/tests/vm/dart_2/regress_43464_test.dart new file mode 100644 index 00000000000..43a3c1135bd --- /dev/null +++ b/runtime/tests/vm/dart_2/regress_43464_test.dart @@ -0,0 +1,21 @@ +// 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. + +import 'package:expect/expect.dart'; + +class A {} + +abstract class B { + dynamic foo(T a); +} + +class C extends B { + dynamic foo(A a) { + return () => a; + } +} + +main() { + Expect.throws(() => (C().foo as dynamic)(1)); +} diff --git a/runtime/vm/compiler/backend/type_propagator.cc b/runtime/vm/compiler/backend/type_propagator.cc index 6c35a83e35b..3877f455625 100644 --- a/runtime/vm/compiler/backend/type_propagator.cc +++ b/runtime/vm/compiler/backend/type_propagator.cc @@ -1405,6 +1405,12 @@ CompileType StaticCallInstr::ComputeType() const { } CompileType LoadLocalInstr::ComputeType() const { + if (local().needs_covariant_check_in_method()) { + // We may not yet have checked the actual type of the parameter value. + // Assuming that the value has the required type can lead to unsound + // optimizations. See dartbug.com/43464. + return CompileType::FromCid(kDynamicCid); + } const AbstractType& local_type = local().type(); TraceStrongModeType(this, local_type); return CompileType::FromAbstractType(local_type); diff --git a/runtime/vm/compiler/frontend/scope_builder.cc b/runtime/vm/compiler/frontend/scope_builder.cc index e736c7ad4be..b859d993580 100644 --- a/runtime/vm/compiler/frontend/scope_builder.cc +++ b/runtime/vm/compiler/frontend/scope_builder.cc @@ -1601,6 +1601,9 @@ void ScopeBuilder::AddVariableDeclarationParameter( helper.IsCovariant() || (helper.IsGenericCovariantImpl() && (attrs.has_non_this_uses || attrs.has_tearoff_uses)); + if (needs_covariant_check_in_method) { + variable->set_needs_covariant_check_in_method(); + } switch (type_check_mode) { case kTypeCheckAllParameters: diff --git a/runtime/vm/parser.cc b/runtime/vm/parser.cc index 9f873d951ad..9902dc644a6 100644 --- a/runtime/vm/parser.cc +++ b/runtime/vm/parser.cc @@ -211,6 +211,9 @@ void ParsedFunction::AllocateVariables() { if (variable->is_explicit_covariant_parameter()) { raw_parameter->set_is_explicit_covariant_parameter(); } + if (variable->needs_covariant_check_in_method()) { + raw_parameter->set_needs_covariant_check_in_method(); + } raw_parameter->set_type_check_mode(variable->type_check_mode()); if (function().HasOptionalParameters()) { bool ok = scope->AddVariable(raw_parameter); diff --git a/runtime/vm/scopes.h b/runtime/vm/scopes.h index a9761d7b336..b0033246eba 100644 --- a/runtime/vm/scopes.h +++ b/runtime/vm/scopes.h @@ -91,7 +91,7 @@ class LocalVariable : public ZoneAllocated { is_invisible_(false), is_captured_parameter_(false), is_forced_stack_(false), - is_explicit_covariant_parameter_(false), + covariance_mode_(kNotCovariant), is_late_(false), is_chained_future_(false), expected_context_index_(-1), @@ -147,10 +147,17 @@ class LocalVariable : public ZoneAllocated { } bool is_explicit_covariant_parameter() const { - return is_explicit_covariant_parameter_; + return covariance_mode_ == kExplicit; } - void set_is_explicit_covariant_parameter() { - is_explicit_covariant_parameter_ = true; + void set_is_explicit_covariant_parameter() { covariance_mode_ = kExplicit; } + + bool needs_covariant_check_in_method() const { + return covariance_mode_ != kNotCovariant; + } + void set_needs_covariant_check_in_method() { + if (covariance_mode_ == kNotCovariant) { + covariance_mode_ = kImplicit; + } } enum TypeCheckMode { @@ -208,6 +215,12 @@ class LocalVariable : public ZoneAllocated { bool Equals(const LocalVariable& other) const; private: + enum CovarianceMode { + kNotCovariant, + kImplicit, + kExplicit, + }; + static const int kUninitializedIndex = INT_MIN; const TokenPosition declaration_pos_; @@ -228,7 +241,7 @@ class LocalVariable : public ZoneAllocated { bool is_invisible_; bool is_captured_parameter_; bool is_forced_stack_; - bool is_explicit_covariant_parameter_; + CovarianceMode covariance_mode_; bool is_late_; bool is_chained_future_; intptr_t expected_context_index_;