From da4ff4e9a695d2eb04dc18af164a0efd0fe1f337 Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Tue, 11 May 2021 18:25:24 +0000 Subject: [PATCH] [vm/aot] Fix handling of annotations in tree shaker Previously, annotations were not traced by global type flow analysis (in order to avoid retaining classes which are only used in annotations). Annotations were only traced during tree shaking and references from such constants were treated much like references from types. This handling of constants in annotations conflicts with removal of fields, as tree shaker needs to know which fields are retained upfront to be independent of the visiting order. In a certain corner case (field was replaced with a getter but was still used in a constant in annotation) that caused incorrect AST and crash during serialization of AST. In order to fix that, this change adds proper tracing through annotations on members, classes and libraries, as if annotation constants were used in the executable code. That also means that annotation classes will be retained as allocated. In order to compensate for that, a new pass is added before the global analysis to clean all annotations except @ExternalName, @pragma (used by the VM) and @TagNumber (used by protobuf tree shaking). TEST=runtime/tests/vm/dart/regress_45968_test.dart Fixes https://github.com/dart-lang/sdk/issues/45968 Change-Id: I998e4f7ec7da7b74e1738fc21b354a4ec9f0c071 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/199200 Commit-Queue: Alexander Markov Reviewed-by: Vyacheslav Egorov Reviewed-by: Martin Kustermann --- .../type_flow/protobuf_handler.dart | 2 + .../type_flow/summary_collector.dart | 6 ++ .../type_flow/transformer.dart | 61 ++++++++++++++++++- .../transformer/annotation.dart.expect | 21 +------ runtime/tests/vm/dart/regress_45968_test.dart | 28 +++++++++ .../tests/vm/dart_2/regress_45968_test.dart | 28 +++++++++ 6 files changed, 125 insertions(+), 21 deletions(-) create mode 100644 runtime/tests/vm/dart/regress_45968_test.dart create mode 100644 runtime/tests/vm/dart_2/regress_45968_test.dart diff --git a/pkg/vm/lib/transformations/type_flow/protobuf_handler.dart b/pkg/vm/lib/transformations/type_flow/protobuf_handler.dart index 865f56d5902..c1f616ddd0a 100644 --- a/pkg/vm/lib/transformations/type_flow/protobuf_handler.dart +++ b/pkg/vm/lib/transformations/type_flow/protobuf_handler.dart @@ -68,6 +68,8 @@ class ProtobufHandler { _builderInfoAddMethod = libraryIndex.getMember(protobufLibraryUri, 'BuilderInfo', 'add'); + bool usesAnnotationClass(Class cls) => cls == _tagNumberClass; + /// This method is called from summary collector when analysis discovered /// that [member] is called and needs to construct a summary for its body. /// diff --git a/pkg/vm/lib/transformations/type_flow/summary_collector.dart b/pkg/vm/lib/transformations/type_flow/summary_collector.dart index a20c8405993..9e73f760dcd 100644 --- a/pkg/vm/lib/transformations/type_flow/summary_collector.dart +++ b/pkg/vm/lib/transformations/type_flow/summary_collector.dart @@ -774,6 +774,12 @@ class SummaryCollector extends RecursiveResultVisitor { _summary.result = _returnValue; } + // Visit annotations on members, classes and libraries. + // Other nodes currently do not have annotations used by the VM. + member.annotations.forEach(_visit); + member.enclosingClass?.annotations?.forEach(_visit); + member.enclosingLibrary?.annotations?.forEach(_visit); + _staticTypeContext = null; debugPrint("------------ SUMMARY ------------"); diff --git a/pkg/vm/lib/transformations/type_flow/transformer.dart b/pkg/vm/lib/transformations/type_flow/transformer.dart index e7ac6527a6c..5dcdfe7f74d 100644 --- a/pkg/vm/lib/transformations/type_flow/transformer.dart +++ b/pkg/vm/lib/transformations/type_flow/transformer.dart @@ -75,6 +75,9 @@ Component transformComponent( typeFlowAnalysis.addRawCall(mainSelector); } + CleanupAnnotations(coreTypes, libraryIndex, protobufHandler) + .visitComponent(component); + typeFlowAnalysis.process(); analysisStopWatch.stop(); @@ -118,6 +121,56 @@ Component transformComponent( return component; } +// Pass which removes all annotations except @ExternalName and @pragma +// on members, classes and libraries. May also keep @TagNumber which is used +// by protobuf handler. +class CleanupAnnotations extends RecursiveVisitor { + final Class externalNameClass; + final Class pragmaClass; + final ProtobufHandler protobufHandler; + + CleanupAnnotations( + CoreTypes coreTypes, LibraryIndex index, this.protobufHandler) + : externalNameClass = index.getClass('dart:_internal', 'ExternalName'), + pragmaClass = coreTypes.pragmaClass; + + @override + defaultNode(Node node) { + if (node is Annotatable && node.annotations.isNotEmpty) { + _cleanupAnnotations(node, node.annotations); + } + super.defaultNode(node); + } + + @override + visitTypedef(Typedef node) { + super.visitTypedef(node); + // These sub-nodes can have annotations but are not visited by + // Typedef.visitChildren. + visitList(node.positionalParameters, this); + visitList(node.namedParameters, this); + } + + void _cleanupAnnotations(Node node, List annotations) { + if (node is Member || node is Class || node is Library) { + annotations.removeWhere((a) => !_keepAnnotation(a)); + } else { + annotations.clear(); + } + } + + bool _keepAnnotation(Expression annotation) { + final constant = (annotation as ConstantExpression).constant; + if (constant is InstanceConstant) { + final cls = constant.classNode; + return (cls == externalNameClass) || + (cls == pragmaClass) || + (protobufHandler != null && protobufHandler.usesAnnotationClass(cls)); + } + return false; + } +} + /// Devirtualization based on results of type flow analysis. class TFADevirtualization extends Devirtualization { final TypeFlowAnalysis _typeFlowAnalysis; @@ -731,6 +784,7 @@ class TreeShaker { void addUsedExtension(Extension node) { if (_usedExtensions.add(node)) { + node.annotations = const []; _pass1.transformTypeParameterList(node.typeParameters, node); node.onType?.accept(typeVisitor); } @@ -738,7 +792,7 @@ class TreeShaker { void addUsedTypedef(Typedef typedef) { if (_usedTypedefs.add(typedef)) { - _pass1.transformExpressionList(typedef.annotations, typedef); + typedef.annotations = const []; _pass1.transformTypeParameterList(typedef.typeParameters, typedef); _pass1.transformTypeParameterList( typedef.typeParametersOfFunctionType, typedef); @@ -1719,7 +1773,10 @@ class _TreeShakerConstantVisitor extends ConstantVisitor { shaker.addClassUsedInType(constant.classNode); visitList(constant.typeArguments, typeVisitor); constant.fieldValues.forEach((Reference fieldRef, Constant value) { - shaker.addUsedMember(fieldRef.asField); + if (!shaker.retainField(fieldRef.asField)) { + throw 'Constant $constant references field ${fieldRef.asField} ' + 'which is not retained'; + } analyzeConstant(value); }); } diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/annotation.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/annotation.dart.expect index 28378bee51e..a1350daa1fd 100644 --- a/pkg/vm/testcases/transformations/type_flow/transformer/annotation.dart.expect +++ b/pkg/vm/testcases/transformations/type_flow/transformer/annotation.dart.expect @@ -2,37 +2,20 @@ library #lib; import self as self; import "dart:core" as core; -@#C5 typedef SomeType = (core::List*) →* void; -abstract class ClassAnnotation2 extends core::Object /*hasConstConstructor*/ { -} -abstract class MethodAnnotation extends core::Object /*hasConstConstructor*/ { -[@vm.unreachable.metadata=] [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasNonThisUses:false,hasTearOffUses:false,getterSelectorId:1] final field core::int* x; -} -abstract class TypedefAnnotation extends core::Object /*hasConstConstructor*/ { -[@vm.unreachable.metadata=] [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasNonThisUses:false,hasTearOffUses:false,getterSelectorId:2] final field core::List* list; -} -abstract class VarAnnotation extends core::Object /*hasConstConstructor*/ { -} -abstract class ParametrizedAnnotation extends core::Object /*hasConstConstructor*/ { -[@vm.unreachable.metadata=] [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasNonThisUses:false,hasTearOffUses:false,getterSelectorId:3] final field self::ParametrizedAnnotation::T* foo; -} abstract class A extends core::Object { static method staticMethod() → void {} } -@#C6 class B extends core::Object { synthetic constructor •() → self::B* : super core::Object::•() ; -[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:4,getterSelectorId:5] @#C8 - method instanceMethod() → void {} +[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:1,getterSelectorId:2] method instanceMethod() → void {} } [@vm.unboxing-info.metadata=()->i]static method foo() → core::int* { - @#C9 core::int* x = 2; + core::int* x = 2; return [@vm.direct-call.metadata=dart.core::_IntegerImplementation.+] [@vm.inferred-type.metadata=int (skip check)] x.{core::num::+}(2); } -@#C11 static method main(core::List* args) → dynamic { self::A::staticMethod(); [@vm.direct-call.metadata=#lib::B.instanceMethod] [@vm.inferred-type.metadata=!? (skip check)] new self::B::•().{self::B::instanceMethod}(); diff --git a/runtime/tests/vm/dart/regress_45968_test.dart b/runtime/tests/vm/dart/regress_45968_test.dart new file mode 100644 index 00000000000..80cff1fb7c9 --- /dev/null +++ b/runtime/tests/vm/dart/regress_45968_test.dart @@ -0,0 +1,28 @@ +// Copyright (c) 2021, 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/dart-lang/sdk/issues/45968. +// Verifies that compiler doesn't crash if annotation references field +// which is replaced with a getter. + +class Qualifier { + final String name; + const Qualifier(this.name); +} + +class Foo implements Qualifier { + String get name => 'a'; +} + +class Bar { + @Qualifier('b') + void bar() {} +} + +Qualifier x = Foo(); + +main() { + print(x.name); + Bar().bar(); +} diff --git a/runtime/tests/vm/dart_2/regress_45968_test.dart b/runtime/tests/vm/dart_2/regress_45968_test.dart new file mode 100644 index 00000000000..80cff1fb7c9 --- /dev/null +++ b/runtime/tests/vm/dart_2/regress_45968_test.dart @@ -0,0 +1,28 @@ +// Copyright (c) 2021, 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/dart-lang/sdk/issues/45968. +// Verifies that compiler doesn't crash if annotation references field +// which is replaced with a getter. + +class Qualifier { + final String name; + const Qualifier(this.name); +} + +class Foo implements Qualifier { + String get name => 'a'; +} + +class Bar { + @Qualifier('b') + void bar() {} +} + +Qualifier x = Foo(); + +main() { + print(x.name); + Bar().bar(); +}