[vm/compiler] Fix computation of ParameterInstr type in a catch block

In order to compute type, ParameterInstr::ComputeType() uses
environment index to get a LocalVariable from LocalScope,
assuming that environment index matches a variable index in the scope.
This is only true for direct parameters (which are not copied in
prologue).

This change limits use of LocalVariable type for ParameterInstr
corresponding to direct parameters. Note that it only affects
Parameter instructions used in catch block entries, as
ParameterInstr in function entry always corresponds to a direct
parameter.

TEST=runtime/tests/vm/dart/regress_flutter110715_il_test.dart
Fixes flutter/flutter#110715

Change-Id: I68d423860928d7e65143844522e3006d9ccfcf66
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/257441
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>

runtime/tests/vm/dart/regress_flutter110715_il_test.dart

@@ -0,0 +1,96 @@
+// Copyright (c) 2022, 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/110715.
+// Verifies that compiler doesn't elide null check for a parameter in
+// a catch block in async/async* methods.
+
+import 'dart:async';
+import 'package:vm/testing/il_matchers.dart';
+
+@pragma('vm:never-inline')
+@pragma('vm:testing:print-flow-graph')
+Stream<Object> bug1(void Function()? f, void Function() g) async* {
+  try {
+    g();
+    throw 'error';
+  } catch (e) {
+    // Should not crash when 'f' is null.
+    f?.call();
+  }
+}
+
+@pragma('vm:never-inline')
+@pragma('vm:testing:print-flow-graph')
+Future<Object> bug2(void Function()? f, void Function() g) async {
+  try {
+    g();
+    throw 'error';
+  } catch (e) {
+    // Should not crash when 'f' is null.
+    f?.call();
+  }
+  return '';
+}
+
+void main() async {
+  print(await bug1(null, () {}).toList());
+  print(await bug1(() {}, () {}).toList());
+  print(await bug2(null, () {}));
+  print(await bug2(() {}, () {}));
+}
+
+void matchIL$bug1(FlowGraph graph) {
+  graph.dump();
+  graph.match([
+    match.block('Graph'),
+    match.block('Function'),
+    match.block('Join'),
+    match.block('CatchBlock', [
+      'v0' << match.Parameter(),
+      match.Branch(match.StrictCompare('v0', match.any, kind: '==='),
+          ifTrue: 'B6', ifFalse: 'B7'),
+    ]),
+    'B6' <<
+        match.block('Target', [
+          match.Goto('B4'),
+        ]),
+    'B7' <<
+        match.block('Target', [
+          match.ClosureCall(),
+          match.Goto('B4'),
+        ]),
+    'B4' <<
+        match.block('Join', [
+          match.Return(),
+        ]),
+  ]);
+}
+
+void matchIL$bug2(FlowGraph graph) {
+  graph.dump();
+  graph.match([
+    match.block('Graph'),
+    match.block('Function'),
+    match.block('Join'),
+    match.block('CatchBlock', [
+      'v0' << match.Parameter(),
+      match.Branch(match.StrictCompare('v0', match.any, kind: '==='),
+          ifTrue: 'B6', ifFalse: 'B7'),
+    ]),
+    'B6' <<
+        match.block('Target', [
+          match.Goto('B4'),
+        ]),
+    'B7' <<
+        match.block('Target', [
+          match.ClosureCall(),
+          match.Goto('B4'),
+        ]),
+    'B4' <<
+        match.block('Join', [
+          match.Return(),
+        ]),
+  ]);
+}

runtime/vm/compiler/backend/constant_propagator_test.cc

@@ -117,16 +117,8 @@ static void ConstantPropagatorUnboxedOpTest(
   using compiler::BlockBuilder;
 
   CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
-  FlowGraphBuilderHelper H;
-
-  // Add a variable into the scope which would provide static type for the
-  // parameter.
-  LocalVariable* v0_var =
-      new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
-                        String::Handle(Symbols::New(thread, "v0")),
-                        AbstractType::ZoneHandle(Type::IntType()));
-  v0_var->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
-  H.flow_graph()->parsed_function().scope()->AddVariable(v0_var);
+  FlowGraphBuilderHelper H(/*num_parameters=*/1);
+  H.AddVariable("v0", AbstractType::ZoneHandle(Type::IntType()));
 
   // We are going to build the following graph:
   //

runtime/vm/compiler/backend/flow_graph_test.cc

@@ -22,17 +22,9 @@ ISOLATE_UNIT_TEST_CASE(FlowGraph_UnboxInt64Phi) {
 
   CompilerState S(thread, /*is_aot=*/true, /*is_optimizing=*/true);
 
-  FlowGraphBuilderHelper H;
-
-  // Add a variable into the scope which would provide static type for the
-  // parameter.
-  LocalVariable* v0_var =
-      new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
-                        String::Handle(Symbols::New(thread, "v0")),
-                        AbstractType::ZoneHandle(Type::IntType()),
-                        new CompileType(CompileType::Int()));
-  v0_var->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
-  H.flow_graph()->parsed_function().scope()->AddVariable(v0_var);
+  FlowGraphBuilderHelper H(/*num_parameters=*/1);
+  H.AddVariable("v0", AbstractType::ZoneHandle(Type::IntType()),
+                new CompileType(CompileType::Int()));
 
   auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
   auto loop_header = H.JoinEntry();

runtime/vm/compiler/backend/il.h

@@ -2696,8 +2696,8 @@ class ParameterInstr : public Definition {
   virtual Representation representation() const { return representation_; }
 
   virtual Representation RequiredInputRepresentation(intptr_t index) const {
-    ASSERT(index == 0);
-    return representation();
+    UNREACHABLE();
+    return kTagged;
   }
 
   virtual bool ComputeCanDeoptimize() const { return false; }

runtime/vm/compiler/backend/il_test.cc

@@ -207,16 +207,8 @@ bool TestIntConverterCanonicalizationRule(Thread* thread,
 
   CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
 
-  FlowGraphBuilderHelper H;
-
-  // Add a variable into the scope which would provide static type for the
-  // parameter.
-  LocalVariable* v0_var =
-      new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
-                        String::Handle(Symbols::New(thread, "v0")),
-                        AbstractType::ZoneHandle(Type::IntType()));
-  v0_var->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
-  H.flow_graph()->parsed_function().scope()->AddVariable(v0_var);
+  FlowGraphBuilderHelper H(/*num_parameters=*/1);
+  H.AddVariable("v0", AbstractType::ZoneHandle(Type::IntType()));
 
   auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
 
@@ -269,7 +261,8 @@ ISOLATE_UNIT_TEST_CASE(IL_PhiCanonicalization) {
 
   CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
 
-  FlowGraphBuilderHelper H;
+  FlowGraphBuilderHelper H(/*num_parameters=*/1);
+  H.AddVariable("v0", AbstractType::ZoneHandle(Type::DynamicType()));
 
   auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
   auto b2 = H.JoinEntry();
@@ -322,7 +315,9 @@ ISOLATE_UNIT_TEST_CASE(IL_UnboxIntegerCanonicalization) {
 
   CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
 
-  FlowGraphBuilderHelper H;
+  FlowGraphBuilderHelper H(/*num_parameters=*/2);
+  H.AddVariable("v0", AbstractType::ZoneHandle(Type::DynamicType()));
+  H.AddVariable("v1", AbstractType::ZoneHandle(Type::DynamicType()));
 
   auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
   Definition* unbox;
@@ -397,7 +392,9 @@ static void TestNullAwareEqualityCompareCanonicalization(
 
   CompilerState S(thread, /*is_aot=*/true, /*is_optimizing=*/true);
 
-  FlowGraphBuilderHelper H;
+  FlowGraphBuilderHelper H(/*num_parameters=*/2);
+  H.AddVariable("v0", AbstractType::ZoneHandle(Type::IntType()));
+  H.AddVariable("v1", AbstractType::ZoneHandle(Type::IntType()));
 
   auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
 

runtime/vm/compiler/backend/il_test_helper.h

@@ -261,9 +261,9 @@ class ILMatcher : public ValueObject {
 
 class FlowGraphBuilderHelper {
  public:
-  FlowGraphBuilderHelper()
+  explicit FlowGraphBuilderHelper(intptr_t num_parameters = 0)
       : state_(CompilerState::Current()),
-        flow_graph_(MakeDummyGraph(Thread::Current())) {
+        flow_graph_(MakeDummyGraph(Thread::Current(), num_parameters)) {
     flow_graph_.CreateCommonConstants();
   }
 
@@ -286,6 +286,19 @@ class FlowGraphBuilderHelper {
     return flow_graph_.GetConstant(Double::Handle(Double::NewCanonical(value)));
   }
 
+  // Adds a variable into the scope which would provide static type for the
+  // parameter.
+  void AddVariable(const char* name,
+                   const AbstractType& static_type,
+                   CompileType* param_type = nullptr) {
+    LocalVariable* v =
+        new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
+                          String::Handle(Symbols::New(Thread::Current(), name)),
+                          static_type, param_type);
+    v->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
+    flow_graph()->parsed_function().scope()->AddVariable(v);
+  }
+
   enum class IncomingDefKind {
     kImmediate,
     kDelayed,
@@ -349,9 +362,10 @@ class FlowGraphBuilderHelper {
   FlowGraph* flow_graph() { return &flow_graph_; }
 
  private:
-  static FlowGraph& MakeDummyGraph(Thread* thread) {
+  static FlowGraph& MakeDummyGraph(Thread* thread, intptr_t num_parameters) {
     const FunctionType& signature =
         FunctionType::ZoneHandle(FunctionType::New());
+    signature.set_num_fixed_parameters(num_parameters);
     const Function& func = Function::ZoneHandle(Function::New(
         signature, String::Handle(Symbols::New(thread, "dummy")),
         UntaggedFunction::kRegularFunction,

runtime/vm/compiler/backend/redundancy_elimination_test.cc

@@ -1381,7 +1381,9 @@ ISOLATE_UNIT_TEST_CASE(CSE_Redefinitions) {
 
   using compiler::BlockBuilder;
   CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
-  FlowGraphBuilderHelper H;
+  FlowGraphBuilderHelper H(/*num_parameters=*/2);
+  H.AddVariable("v0", AbstractType::ZoneHandle(Type::DynamicType()));
+  H.AddVariable("v1", AbstractType::ZoneHandle(Type::DynamicType()));
 
   auto b1 = H.flow_graph()->graph_entry()->normal_entry();
 

runtime/vm/compiler/backend/type_propagator.cc

@@ -1176,7 +1176,8 @@ CompileType ParameterInstr::ComputeType() const {
     return CompileType::Dynamic();
   }
 
-  const Function& function = graph_entry->parsed_function().function();
+  const ParsedFunction& pf = graph_entry->parsed_function();
+  const Function& function = pf.function();
   if (function.IsIrregexpFunction()) {
     // In irregexp functions, types of input parameters are known and immutable.
     // Set parameter types here in order to prevent unnecessary CheckClassInstr
@@ -1195,64 +1196,72 @@ CompileType ParameterInstr::ComputeType() const {
     return CompileType::Dynamic();
   }
 
-  // Parameter is the receiver.
-  if ((index() == 0) &&
-      (function.IsDynamicFunction() || function.IsGenerativeConstructor())) {
-    const AbstractType& type =
-        graph_entry->parsed_function().RawParameterVariable(0)->type();
-    if (type.IsObjectType() || type.IsNullType()) {
-      // Receiver can be null.
-      return CompileType::FromAbstractType(type, CompileType::kCanBeNull,
-                                           CompileType::kCannotBeSentinel);
-    }
+  // Figure out if this Parameter instruction corresponds to a direct
+  // parameter. See FlowGraph::EnvIndex and initialization of
+  // num_direct_parameters_ in FlowGraph constructor.
+  const bool is_direct_parameter =
+      !function.MakesCopyOfParameters() && (index() < function.NumParameters());
+  // Parameter instructions in a function entry are only used for direct
+  // parameters. Parameter instructions in OsrEntry and CatchBlockEntry
+  // correspond to all local variables, not just direct parameters.
+  // OsrEntry is already checked above.
+  ASSERT(is_direct_parameter || block_->IsCatchBlockEntry());
+
+  // The code below assumes that env index matches parameter index.
+  // This is true only for direct parameters.
+  if (is_direct_parameter) {
+    const intptr_t param_index = index();
+    // Parameter is the receiver.
+    if ((param_index == 0) &&
+        (function.IsDynamicFunction() || function.IsGenerativeConstructor())) {
+      const AbstractType& type = pf.RawParameterVariable(0)->type();
+      if (type.IsObjectType() || type.IsNullType()) {
+        // Receiver can be null.
+        return CompileType::FromAbstractType(type, CompileType::kCanBeNull,
+                                             CompileType::kCannotBeSentinel);
+      }
 
-    // Receiver can't be null but can be an instance of a subclass.
-    intptr_t cid = kDynamicCid;
+      // Receiver can't be null but can be an instance of a subclass.
+      intptr_t cid = kDynamicCid;
 
-    if (type.type_class_id() != kIllegalCid) {
-      Thread* thread = Thread::Current();
-      const Class& type_class = Class::Handle(type.type_class());
-      if (!CHA::HasSubclasses(type_class)) {
-        if (type_class.IsPrivate()) {
-          // Private classes can never be subclassed by later loaded libs.
-          cid = type_class.id();
-        } else {
-          if (FLAG_use_cha_deopt ||
-              thread->isolate_group()->all_classes_finalized()) {
-            if (FLAG_trace_cha) {
-              THR_Print(
-                  "  **(CHA) Computing exact type of receiver, "
-                  "no subclasses: %s\n",
-                  type_class.ToCString());
-            }
-            if (FLAG_use_cha_deopt) {
-              thread->compiler_state().cha().AddToGuardedClasses(
-                  type_class,
-                  /*subclass_count=*/0);
-            }
+      if (type.type_class_id() != kIllegalCid) {
+        Thread* thread = Thread::Current();
+        const Class& type_class = Class::Handle(type.type_class());
+        if (!CHA::HasSubclasses(type_class)) {
+          if (type_class.IsPrivate()) {
+            // Private classes can never be subclassed by later loaded libs.
             cid = type_class.id();
+          } else {
+            if (FLAG_use_cha_deopt ||
+                thread->isolate_group()->all_classes_finalized()) {
+              if (FLAG_trace_cha) {
+                THR_Print(
+                    "  **(CHA) Computing exact type of receiver, "
+                    "no subclasses: %s\n",
+                    type_class.ToCString());
+              }
+              if (FLAG_use_cha_deopt) {
+                thread->compiler_state().cha().AddToGuardedClasses(
+                    type_class,
+                    /*subclass_count=*/0);
+              }
+              cid = type_class.id();
+            }
           }
         }
       }
-    }
 
-    return CompileType(CompileType::kCannotBeNull,
-                       CompileType::kCannotBeSentinel, cid, &type);
-  }
+      return CompileType(CompileType::kCannotBeNull,
+                         CompileType::kCannotBeSentinel, cid, &type);
+    }
 
-  const bool is_unchecked_entry_param =
-      graph_entry->unchecked_entry() == block_;
+    const bool is_unchecked_entry_param =
+        graph_entry->unchecked_entry() == block_;
 
-  LocalScope* scope = graph_entry->parsed_function().scope();
-  // Note: in catch-blocks we have ParameterInstr for each local variable
-  // not only for normal parameters.
-  const LocalVariable* param = nullptr;
-  if (scope != nullptr && (index() < scope->num_variables())) {
-    param = scope->VariableAt(index());
-  } else if (index() < function.NumParameters()) {
-    param = graph_entry->parsed_function().RawParameterVariable(index());
-  }
-  if (param != nullptr) {
+    const LocalVariable* param = (pf.scope() != nullptr)
+                                     ? pf.ParameterVariable(param_index)
+                                     : pf.RawParameterVariable(param_index);
+    ASSERT(param != nullptr);
     CompileType* inferred_type = NULL;
     if (!block_->IsCatchBlockEntry()) {
       inferred_type = param->parameter_type();