From 022b362e848020137704df11071d1a5d8a298776 Mon Sep 17 00:00:00 2001 From: Aske Simon Christensen Date: Thu, 13 Feb 2020 15:32:48 +0000 Subject: [PATCH] [vm/aot/tfa] Add missing visitor methods for call site instrumentation. Some visitor methods were missing in the TFA transformation, causing some call sites (such as PropertySet) to not be instrumented with metadata communicating the analysis results. Visiting all instance call sites (including PropertySet) is also needed by table dispatch to collect correct summary information on selectors. Change-Id: I488d5cd10700666dab05bd5c5304010aa90b1943 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/135319 Reviewed-by: Samir Jindel Reviewed-by: Alexander Markov --- .../transformations/type_flow/analysis.dart | 6 +++++ .../type_flow/transformer.dart | 24 +++++++++++++++++++ .../invalidation_set_field.dart.expect | 2 +- .../regress_flutter16182.dart.expect | 8 +++---- .../transformer/write_only_field.dart.expect | 2 +- .../write_only_field3_nnbd.dart.expect | 4 ++-- 6 files changed, 38 insertions(+), 8 deletions(-) diff --git a/pkg/vm/lib/transformations/type_flow/analysis.dart b/pkg/vm/lib/transformations/type_flow/analysis.dart index ec56e61948b..62ce853b1ab 100644 --- a/pkg/vm/lib/transformations/type_flow/analysis.dart +++ b/pkg/vm/lib/transformations/type_flow/analysis.dart @@ -162,6 +162,12 @@ class _DirectInvocation extends _Invocation { if (selector.member.function != null) { typeChecksNeeded = selector.member.function.typeParameters .any((t) => t.isGenericCovariantImpl); + } else { + Field field = selector.member; + if (selector.callKind == CallKind.PropertySet) { + // TODO(dartbug.com/40615): Use TFA results to improve this criterion. + typeChecksNeeded = field.isGenericCovariantImpl; + } } } diff --git a/pkg/vm/lib/transformations/type_flow/transformer.dart b/pkg/vm/lib/transformations/type_flow/transformer.dart index 5f20ca61d9c..efbd3ff6b29 100644 --- a/pkg/vm/lib/transformations/type_flow/transformer.dart +++ b/pkg/vm/lib/transformations/type_flow/transformer.dart @@ -340,6 +340,12 @@ class AnnotateKernel extends RecursiveVisitor { super.visitPropertyGet(node); } + @override + visitPropertySet(PropertySet node) { + _annotateCallSite(node, node.interfaceTarget); + super.visitPropertySet(node); + } + @override visitDirectMethodInvocation(DirectMethodInvocation node) { _annotateCallSite(node, node.target); @@ -352,6 +358,12 @@ class AnnotateKernel extends RecursiveVisitor { super.visitDirectPropertyGet(node); } + @override + visitDirectPropertySet(DirectPropertySet node) { + _annotateCallSite(node, node.target); + super.visitDirectPropertySet(node); + } + @override visitSuperMethodInvocation(SuperMethodInvocation node) { _annotateCallSite(node, node.interfaceTarget); @@ -364,6 +376,12 @@ class AnnotateKernel extends RecursiveVisitor { super.visitSuperPropertyGet(node); } + @override + visitSuperPropertySet(SuperPropertySet node) { + _annotateCallSite(node, node.interfaceTarget); + super.visitSuperPropertySet(node); + } + @override visitStaticInvocation(StaticInvocation node) { _annotateCallSite(node, node.target); @@ -375,6 +393,12 @@ class AnnotateKernel extends RecursiveVisitor { _annotateCallSite(node, node.target); super.visitStaticGet(node); } + + @override + visitStaticSet(StaticSet node) { + _annotateCallSite(node, node.target); + super.visitStaticSet(node); + } } /// Tree shaking based on results of type flow analysis (TFA). diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/invalidation_set_field.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/invalidation_set_field.dart.expect index 02184b823a7..6698bb7ae12 100644 --- a/pkg/vm/testcases/transformations/type_flow/transformer/invalidation_set_field.dart.expect +++ b/pkg/vm/testcases/transformations/type_flow/transformer/invalidation_set_field.dart.expect @@ -53,7 +53,7 @@ static method use2([@vm.inferred-type.metadata=#lib::DeepCaller2] self::DeepCall static method getDynamic() → dynamic return [@vm.call-site-attributes.metadata=receiverType:dart.core::Function*] self::unknown.call(); static method setField2([@vm.inferred-type.metadata=#lib::A] self::A* aa, [@vm.inferred-type.metadata=#lib::T2] dynamic value) → void { - [@vm.direct-call.metadata=#lib::A::field2] aa.{self::A::field2} = value; + [@vm.direct-call.metadata=#lib::A::field2] [@vm.inferred-type.metadata=!? (skip check)] aa.{self::A::field2} = value; } static method main(core::List* args) → dynamic { new self::A::•(); diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/regress_flutter16182.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/regress_flutter16182.dart.expect index 4f0ebc65510..17d02d46c59 100644 --- a/pkg/vm/testcases/transformations/type_flow/transformer/regress_flutter16182.dart.expect +++ b/pkg/vm/testcases/transformations/type_flow/transformer/regress_flutter16182.dart.expect @@ -20,7 +20,7 @@ class A1 extends core::Object { : super core::Object::•() ; [@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4] method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=#lib::T1?] dynamic a5 = #C1]) → void { - [@vm.direct-call.metadata=#lib::A1::foo] this.{self::A1::foo} = _in::unsafeCast(a5); + [@vm.direct-call.metadata=#lib::A1::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A1::foo} = _in::unsafeCast(a5); } } class B1 extends core::Object { @@ -43,7 +43,7 @@ class A2 extends core::Object { : super core::Object::•() ; [@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4] method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=#lib::T2?] dynamic a6 = #C1]) → void { - [@vm.direct-call.metadata=#lib::A2::foo] this.{self::A2::foo} = a6; + [@vm.direct-call.metadata=#lib::A2::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A2::foo} = a6; } } abstract class B2Base extends core::Object { @@ -76,7 +76,7 @@ class A3 extends core::Object { : super core::Object::•() ; [@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4] method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a6 = #C1, [@vm.inferred-type.metadata=#lib::T3?] dynamic a7 = #C1]) → void { - [@vm.direct-call.metadata=#lib::A3::foo] this.{self::A3::foo} = a7; + [@vm.direct-call.metadata=#lib::A3::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A3::foo} = a7; } } class B3 extends core::Object { @@ -99,7 +99,7 @@ class A4 extends core::Object { : super core::Object::•() ; [@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4] method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a6 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a7 = #C1, [@vm.inferred-type.metadata=#lib::T4?] dynamic a8 = #C1]) → void { - [@vm.direct-call.metadata=#lib::A4::foo] this.{self::A4::foo} = a8; + [@vm.direct-call.metadata=#lib::A4::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A4::foo} = a8; } } class B4 extends core::Object { diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field.dart.expect index 7acac49570a..e84d6a43b2f 100644 --- a/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field.dart.expect +++ b/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field.dart.expect @@ -16,5 +16,5 @@ class C extends core::Object { } static method main() → void { null; - [@vm.direct-call.metadata=#lib::C::instanceField] new self::C::•().{self::C::instanceField} = null; + [@vm.direct-call.metadata=#lib::C::instanceField] [@vm.inferred-type.metadata=!? (skip check)] new self::C::•().{self::C::instanceField} = null; } diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field3_nnbd.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field3_nnbd.dart.expect index b2e47a86388..b234a3590e2 100644 --- a/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field3_nnbd.dart.expect +++ b/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field3_nnbd.dart.expect @@ -9,7 +9,7 @@ class A extends core::Object { : super core::Object::•() ; [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:1,getterSelectorId:2] method use() → dynamic { - [@vm.direct-call.metadata=#lib::A::x] this.{self::A::x} = 3; + [@vm.direct-call.metadata=#lib::A::x] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A::x} = 3; } [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasNonThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3] set /*isNullableByDefault*/ x(core::int value) → void; } @@ -19,7 +19,7 @@ class B extends core::Object { : super core::Object::•() ; [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:1,getterSelectorId:2] method use() → dynamic { - this.{self::B::x} = 3; + [@vm.inferred-type.metadata=!? (skip check)] this.{self::B::x} = 3; } } [@vm.inferred-type.metadata=dart.core::_Smi?]late static final field core::int staticLateB;