[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 #43464

Some considerations for a cleaner fix are described in
#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 <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: askeksa

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<T> {
+  dynamic foo(T a);
+}
+
+class C extends B<A> {
+  dynamic foo(A a) {
+    return () => a;
+  }
+}
+
+main() {
+  Expect.throws(() => (C().foo as dynamic)(1));
+}

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<T> {
+  dynamic foo(T a);
+}
+
+class C extends B<A> {
+  dynamic foo(A a) {
+    return () => a;
+  }
+}
+
+main() {
+  Expect.throws(() => (C().foo as dynamic)(1));
+}

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);

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:

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);

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_;