Skip to content

Commit f38a280

Browse files
alexmarkovCommit Bot
alexmarkov
authored and
Commit Bot
committed
[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 <[email protected]> Reviewed-by: Slava Egorov <[email protected]>
1 parent eb54019 commit f38a280

File tree

9 files changed

+203
-114
lines changed

9 files changed

+203
-114
lines changed

‎runtime/tests/vm/dart/regress_flutter110715_il_test.dart

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// Regression test for https://github.com/flutter/flutter/issues/110715.
6+
// Verifies that compiler doesn't elide null check for a parameter in
7+
// a catch block in async/async* methods.
8+
9+
import 'dart:async';
10+
import 'package:vm/testing/il_matchers.dart';
11+
12+
@pragma('vm:never-inline')
13+
@pragma('vm:testing:print-flow-graph')
14+
Stream<Object> bug1(void Function()? f, void Function() g) async* {
15+
try {
16+
g();
17+
throw 'error';
18+
} catch (e) {
19+
// Should not crash when 'f' is null.
20+
f?.call();
21+
}
22+
}
23+
24+
@pragma('vm:never-inline')
25+
@pragma('vm:testing:print-flow-graph')
26+
Future<Object> bug2(void Function()? f, void Function() g) async {
27+
try {
28+
g();
29+
throw 'error';
30+
} catch (e) {
31+
// Should not crash when 'f' is null.
32+
f?.call();
33+
}
34+
return '';
35+
}
36+
37+
void main() async {
38+
print(await bug1(null, () {}).toList());
39+
print(await bug1(() {}, () {}).toList());
40+
print(await bug2(null, () {}));
41+
print(await bug2(() {}, () {}));
42+
}
43+
44+
void matchIL$bug1(FlowGraph graph) {
45+
graph.dump();
46+
graph.match([
47+
match.block('Graph'),
48+
match.block('Function'),
49+
match.block('Join'),
50+
match.block('CatchBlock', [
51+
'v0' << match.Parameter(),
52+
match.Branch(match.StrictCompare('v0', match.any, kind: '==='),
53+
ifTrue: 'B6', ifFalse: 'B7'),
54+
]),
55+
'B6' <<
56+
match.block('Target', [
57+
match.Goto('B4'),
58+
]),
59+
'B7' <<
60+
match.block('Target', [
61+
match.ClosureCall(),
62+
match.Goto('B4'),
63+
]),
64+
'B4' <<
65+
match.block('Join', [
66+
match.Return(),
67+
]),
68+
]);
69+
}
70+
71+
void matchIL$bug2(FlowGraph graph) {
72+
graph.dump();
73+
graph.match([
74+
match.block('Graph'),
75+
match.block('Function'),
76+
match.block('Join'),
77+
match.block('CatchBlock', [
78+
'v0' << match.Parameter(),
79+
match.Branch(match.StrictCompare('v0', match.any, kind: '==='),
80+
ifTrue: 'B6', ifFalse: 'B7'),
81+
]),
82+
'B6' <<
83+
match.block('Target', [
84+
match.Goto('B4'),
85+
]),
86+
'B7' <<
87+
match.block('Target', [
88+
match.ClosureCall(),
89+
match.Goto('B4'),
90+
]),
91+
'B4' <<
92+
match.block('Join', [
93+
match.Return(),
94+
]),
95+
]);
96+
}

‎runtime/vm/compiler/backend/constant_propagator_test.cc

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -117,16 +117,8 @@ static void ConstantPropagatorUnboxedOpTest(
117117
using compiler::BlockBuilder;
118118

119119
CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
120-
FlowGraphBuilderHelper H;
121-
122-
// Add a variable into the scope which would provide static type for the
123-
// parameter.
124-
LocalVariable* v0_var =
125-
new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
126-
String::Handle(Symbols::New(thread, "v0")),
127-
AbstractType::ZoneHandle(Type::IntType()));
128-
v0_var->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
129-
H.flow_graph()->parsed_function().scope()->AddVariable(v0_var);
120+
FlowGraphBuilderHelper H(/*num_parameters=*/1);
121+
H.AddVariable("v0", AbstractType::ZoneHandle(Type::IntType()));
130122

131123
// We are going to build the following graph:
132124
//

‎runtime/vm/compiler/backend/flow_graph_test.cc

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,17 +23,9 @@ ISOLATE_UNIT_TEST_CASE(FlowGraph_UnboxInt64Phi) {
2323

2424
CompilerState S(thread, /*is_aot=*/true, /*is_optimizing=*/true);
2525

26-
FlowGraphBuilderHelper H;
27-
28-
// Add a variable into the scope which would provide static type for the
29-
// parameter.
30-
LocalVariable* v0_var =
31-
new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
32-
String::Handle(Symbols::New(thread, "v0")),
33-
AbstractType::ZoneHandle(Type::IntType()),
34-
new CompileType(CompileType::Int()));
35-
v0_var->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
36-
H.flow_graph()->parsed_function().scope()->AddVariable(v0_var);
26+
FlowGraphBuilderHelper H(/*num_parameters=*/1);
27+
H.AddVariable("v0", AbstractType::ZoneHandle(Type::IntType()),
28+
new CompileType(CompileType::Int()));
3729

3830
auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
3931
auto loop_header = H.JoinEntry();

‎runtime/vm/compiler/backend/il.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2823,8 +2823,8 @@ class ParameterInstr : public TemplateDefinition<0, NoThrow> {
28232823
virtual Representation representation() const { return representation_; }
28242824

28252825
virtual Representation RequiredInputRepresentation(intptr_t index) const {
2826-
ASSERT(index == 0);
2827-
return representation();
2826+
UNREACHABLE();
2827+
return kTagged;
28282828
}
28292829

28302830
virtual bool ComputeCanDeoptimize() const { return false; }

‎runtime/vm/compiler/backend/il_test.cc

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -207,16 +207,8 @@ bool TestIntConverterCanonicalizationRule(Thread* thread,
207207

208208
CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
209209

210-
FlowGraphBuilderHelper H;
211-
212-
// Add a variable into the scope which would provide static type for the
213-
// parameter.
214-
LocalVariable* v0_var =
215-
new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
216-
String::Handle(Symbols::New(thread, "v0")),
217-
AbstractType::ZoneHandle(Type::IntType()));
218-
v0_var->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
219-
H.flow_graph()->parsed_function().scope()->AddVariable(v0_var);
210+
FlowGraphBuilderHelper H(/*num_parameters=*/1);
211+
H.AddVariable("v0", AbstractType::ZoneHandle(Type::IntType()));
220212

221213
auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
222214

@@ -269,7 +261,8 @@ ISOLATE_UNIT_TEST_CASE(IL_PhiCanonicalization) {
269261

270262
CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
271263

272-
FlowGraphBuilderHelper H;
264+
FlowGraphBuilderHelper H(/*num_parameters=*/1);
265+
H.AddVariable("v0", AbstractType::ZoneHandle(Type::DynamicType()));
273266

274267
auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
275268
auto b2 = H.JoinEntry();
@@ -322,7 +315,9 @@ ISOLATE_UNIT_TEST_CASE(IL_UnboxIntegerCanonicalization) {
322315

323316
CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
324317

325-
FlowGraphBuilderHelper H;
318+
FlowGraphBuilderHelper H(/*num_parameters=*/2);
319+
H.AddVariable("v0", AbstractType::ZoneHandle(Type::DynamicType()));
320+
H.AddVariable("v1", AbstractType::ZoneHandle(Type::DynamicType()));
326321

327322
auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
328323
Definition* unbox;
@@ -397,7 +392,9 @@ static void TestNullAwareEqualityCompareCanonicalization(
397392

398393
CompilerState S(thread, /*is_aot=*/true, /*is_optimizing=*/true);
399394

400-
FlowGraphBuilderHelper H;
395+
FlowGraphBuilderHelper H(/*num_parameters=*/2);
396+
H.AddVariable("v0", AbstractType::ZoneHandle(Type::IntType()));
397+
H.AddVariable("v1", AbstractType::ZoneHandle(Type::IntType()));
401398

402399
auto normal_entry = H.flow_graph()->graph_entry()->normal_entry();
403400

‎runtime/vm/compiler/backend/il_test_helper.h

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -261,9 +261,9 @@ class ILMatcher : public ValueObject {
261261

262262
class FlowGraphBuilderHelper {
263263
public:
264-
FlowGraphBuilderHelper()
264+
explicit FlowGraphBuilderHelper(intptr_t num_parameters = 0)
265265
: state_(CompilerState::Current()),
266-
flow_graph_(MakeDummyGraph(Thread::Current())) {
266+
flow_graph_(MakeDummyGraph(Thread::Current(), num_parameters)) {
267267
flow_graph_.CreateCommonConstants();
268268
}
269269

@@ -286,6 +286,19 @@ class FlowGraphBuilderHelper {
286286
return flow_graph_.GetConstant(Double::Handle(Double::NewCanonical(value)));
287287
}
288288

289+
// Adds a variable into the scope which would provide static type for the
290+
// parameter.
291+
void AddVariable(const char* name,
292+
const AbstractType& static_type,
293+
CompileType* param_type = nullptr) {
294+
LocalVariable* v =
295+
new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
296+
String::Handle(Symbols::New(Thread::Current(), name)),
297+
static_type, param_type);
298+
v->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
299+
flow_graph()->parsed_function().scope()->AddVariable(v);
300+
}
301+
289302
enum class IncomingDefKind {
290303
kImmediate,
291304
kDelayed,
@@ -349,9 +362,10 @@ class FlowGraphBuilderHelper {
349362
FlowGraph* flow_graph() { return &flow_graph_; }
350363

351364
private:
352-
static FlowGraph& MakeDummyGraph(Thread* thread) {
365+
static FlowGraph& MakeDummyGraph(Thread* thread, intptr_t num_parameters) {
353366
const FunctionType& signature =
354367
FunctionType::ZoneHandle(FunctionType::New());
368+
signature.set_num_fixed_parameters(num_parameters);
355369
const Function& func = Function::ZoneHandle(Function::New(
356370
signature, String::Handle(Symbols::New(thread, "dummy")),
357371
UntaggedFunction::kRegularFunction,

‎runtime/vm/compiler/backend/redundancy_elimination_test.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1473,7 +1473,9 @@ ISOLATE_UNIT_TEST_CASE(CSE_Redefinitions) {
14731473

14741474
using compiler::BlockBuilder;
14751475
CompilerState S(thread, /*is_aot=*/false, /*is_optimizing=*/true);
1476-
FlowGraphBuilderHelper H;
1476+
FlowGraphBuilderHelper H(/*num_parameters=*/2);
1477+
H.AddVariable("v0", AbstractType::ZoneHandle(Type::DynamicType()));
1478+
H.AddVariable("v1", AbstractType::ZoneHandle(Type::DynamicType()));
14771479

14781480
auto b1 = H.flow_graph()->graph_entry()->normal_entry();
14791481

‎runtime/vm/compiler/backend/type_propagator.cc

Lines changed: 60 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1158,7 +1158,8 @@ CompileType ParameterInstr::ComputeType() const {
11581158
return CompileType::Dynamic();
11591159
}
11601160

1161-
const Function& function = graph_entry->parsed_function().function();
1161+
const ParsedFunction& pf = graph_entry->parsed_function();
1162+
const Function& function = pf.function();
11621163
if (function.IsIrregexpFunction()) {
11631164
// In irregexp functions, types of input parameters are known and immutable.
11641165
// Set parameter types here in order to prevent unnecessary CheckClassInstr
@@ -1177,64 +1178,72 @@ CompileType ParameterInstr::ComputeType() const {
11771178
return CompileType::Dynamic();
11781179
}
11791180

1180-
// Parameter is the receiver.
1181-
if ((index() == 0) &&
1182-
(function.IsDynamicFunction() || function.IsGenerativeConstructor())) {
1183-
const AbstractType& type =
1184-
graph_entry->parsed_function().RawParameterVariable(0)->type();
1185-
if (type.IsObjectType() || type.IsNullType()) {
1186-
// Receiver can be null.
1187-
return CompileType::FromAbstractType(type, CompileType::kCanBeNull,
1188-
CompileType::kCannotBeSentinel);
1189-
}
1181+
// Figure out if this Parameter instruction corresponds to a direct
1182+
// parameter. See FlowGraph::EnvIndex and initialization of
1183+
// num_direct_parameters_ in FlowGraph constructor.
1184+
const bool is_direct_parameter =
1185+
!function.MakesCopyOfParameters() && (index() < function.NumParameters());
1186+
// Parameter instructions in a function entry are only used for direct
1187+
// parameters. Parameter instructions in OsrEntry and CatchBlockEntry
1188+
// correspond to all local variables, not just direct parameters.
1189+
// OsrEntry is already checked above.
1190+
ASSERT(is_direct_parameter || block_->IsCatchBlockEntry());
1191+
1192+
// The code below assumes that env index matches parameter index.
1193+
// This is true only for direct parameters.
1194+
if (is_direct_parameter) {
1195+
const intptr_t param_index = index();
1196+
// Parameter is the receiver.
1197+
if ((param_index == 0) &&
1198+
(function.IsDynamicFunction() || function.IsGenerativeConstructor())) {
1199+
const AbstractType& type = pf.RawParameterVariable(0)->type();
1200+
if (type.IsObjectType() || type.IsNullType()) {
1201+
// Receiver can be null.
1202+
return CompileType::FromAbstractType(type, CompileType::kCanBeNull,
1203+
CompileType::kCannotBeSentinel);
1204+
}
11901205

1191-
// Receiver can't be null but can be an instance of a subclass.
1192-
intptr_t cid = kDynamicCid;
1193-
1194-
if (type.type_class_id() != kIllegalCid) {
1195-
Thread* thread = Thread::Current();
1196-
const Class& type_class = Class::Handle(type.type_class());
1197-
if (!CHA::HasSubclasses(type_class)) {
1198-
if (type_class.IsPrivate()) {
1199-
// Private classes can never be subclassed by later loaded libs.
1200-
cid = type_class.id();
1201-
} else {
1202-
if (FLAG_use_cha_deopt ||
1203-
thread->isolate_group()->all_classes_finalized()) {
1204-
if (FLAG_trace_cha) {
1205-
THR_Print(
1206-
" **(CHA) Computing exact type of receiver, "
1207-
"no subclasses: %s\n",
1208-
type_class.ToCString());
1209-
}
1210-
if (FLAG_use_cha_deopt) {
1211-
thread->compiler_state().cha().AddToGuardedClasses(
1212-
type_class,
1213-
/*subclass_count=*/0);
1214-
}
1206+
// Receiver can't be null but can be an instance of a subclass.
1207+
intptr_t cid = kDynamicCid;
1208+
1209+
if (type.type_class_id() != kIllegalCid) {
1210+
Thread* thread = Thread::Current();
1211+
const Class& type_class = Class::Handle(type.type_class());
1212+
if (!CHA::HasSubclasses(type_class)) {
1213+
if (type_class.IsPrivate()) {
1214+
// Private classes can never be subclassed by later loaded libs.
12151215
cid = type_class.id();
1216+
} else {
1217+
if (FLAG_use_cha_deopt ||
1218+
thread->isolate_group()->all_classes_finalized()) {
1219+
if (FLAG_trace_cha) {
1220+
THR_Print(
1221+
" **(CHA) Computing exact type of receiver, "
1222+
"no subclasses: %s\n",
1223+
type_class.ToCString());
1224+
}
1225+
if (FLAG_use_cha_deopt) {
1226+
thread->compiler_state().cha().AddToGuardedClasses(
1227+
type_class,
1228+
/*subclass_count=*/0);
1229+
}
1230+
cid = type_class.id();
1231+
}
12161232
}
12171233
}
12181234
}
1219-
}
12201235

1221-
return CompileType(CompileType::kCannotBeNull,
1222-
CompileType::kCannotBeSentinel, cid, &type);
1223-
}
1236+
return CompileType(CompileType::kCannotBeNull,
1237+
CompileType::kCannotBeSentinel, cid, &type);
1238+
}
12241239

1225-
const bool is_unchecked_entry_param =
1226-
graph_entry->unchecked_entry() == block_;
1240+
const bool is_unchecked_entry_param =
1241+
graph_entry->unchecked_entry() == block_;
12271242

1228-
LocalScope* scope = graph_entry->parsed_function().scope();
1229-
// Note: in catch-blocks we have ParameterInstr for each local variable
1230-
// not only for normal parameters.
1231-
const LocalVariable* param = nullptr;
1232-
if (scope != nullptr && (index() < scope->num_variables())) {
1233-
param = scope->VariableAt(index());
1234-
} else if (index() < function.NumParameters()) {
1235-
param = graph_entry->parsed_function().RawParameterVariable(index());
1236-
}
1237-
if (param != nullptr) {
1243+
const LocalVariable* param = (pf.scope() != nullptr)
1244+
? pf.ParameterVariable(param_index)
1245+
: pf.RawParameterVariable(param_index);
1246+
ASSERT(param != nullptr);
12381247
CompileType* inferred_type = NULL;
12391248
if (!block_->IsCatchBlockEntry()) {
12401249
inferred_type = param->parameter_type();

0 commit comments

Comments
 (0)