From 22aa820bd201cb496893241f3ccedee7d7aa19e4 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Mon, 18 May 2020 21:30:16 +0000 Subject: [PATCH] [kernel] Fix round trip serialization of metadata when nodes are lazy loaded. Metadata repository might be populated lazily as we are traversing the component, so it is incorrect to skip empty metadata repositories early in the serialization process. Instead we filter empty repositories at the very end - after all nodes were written out. Change-Id: I159e0c0213a034388855944af03e72786d9e951b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/148065 Commit-Queue: Vyacheslav Egorov Reviewed-by: Jens Johansen --- pkg/kernel/lib/binary/ast_to_binary.dart | 24 +++-- pkg/kernel/test/metadata_test.dart | 124 ++++++++++++++--------- 2 files changed, 92 insertions(+), 56 deletions(-) diff --git a/pkg/kernel/lib/binary/ast_to_binary.dart b/pkg/kernel/lib/binary/ast_to_binary.dart index 069076b20de..553698e781f 100644 --- a/pkg/kernel/lib/binary/ast_to_binary.dart +++ b/pkg/kernel/lib/binary/ast_to_binary.dart @@ -579,16 +579,18 @@ class BinaryPrinter implements Visitor, BinarySink { } } - /// Collect non-empty metadata repositories associated with the component. + /// Collect metadata repositories associated with the component. void _collectMetadata(Component component) { - component.metadata.forEach((tag, repository) { - if (repository.mapping.isEmpty) { - return; - } - - _metadataSubsections ??= <_MetadataSubsection>[]; - _metadataSubsections.add(new _MetadataSubsection(repository)); - }); + if (component.metadata.isNotEmpty) { + // Component might be loaded lazily - meaning that we can't + // just skip empty repositories here, they might be populated by + // the serialization process. Instead we will filter empty repositories + // later before writing the section out. + _metadataSubsections = component.metadata.values + .map((MetadataRepository repository) => + new _MetadataSubsection(repository)) + .toList(); + } } /// Writes metadata associated with the given [Node]. @@ -668,8 +670,10 @@ class BinaryPrinter implements Visitor, BinarySink { } _binaryOffsetForMetadataPayloads = getBufferOffset(); + _metadataSubsections + ?.removeWhere((_MetadataSubsection s) => s.metadataMapping.isEmpty); - if (_metadataSubsections == null) { + if (_metadataSubsections == null || _metadataSubsections.isEmpty) { _binaryOffsetForMetadataMappings = getBufferOffset(); writeUInt32(0); // Empty section. return; diff --git a/pkg/kernel/test/metadata_test.dart b/pkg/kernel/test/metadata_test.dart index a6d285a4e38..dd557fb5866 100644 --- a/pkg/kernel/test/metadata_test.dart +++ b/pkg/kernel/test/metadata_test.dart @@ -87,57 +87,55 @@ class BytesBuilderSink implements Sink> { void close() {} } -/// Visitor that assigns [Metadata] object created with [Metadata.forNode] to -/// each supported node in the component. -class Annotator extends RecursiveVisitor { - final TestMetadataRepository repository; +typedef NodePredicate = bool Function(TreeNode node); - Annotator(Component component) - : repository = component.metadata[TestMetadataRepository.kTag]; +/// Visitor calling [handle] function on every node which can have metadata +/// associated with it and also satisfies the given [predicate]. +class Visitor extends RecursiveVisitor { + final NodePredicate predicate; + final void Function(TreeNode) handle; + + Visitor(this.predicate, this.handle); defaultTreeNode(TreeNode node) { super.defaultTreeNode(node); - if (MetadataRepository.isSupported(node)) { - repository.mapping[node] = new Metadata.forNode(node); + if (MetadataRepository.isSupported(node) && predicate(node)) { + handle(node); } } - - static void annotate(Component p) { - globalDebuggingNames = new NameSystem(); - p.accept(new Annotator(p)); - } } -/// Visitor that checks that each supported node in the component has correct -/// metadata. -class Validator extends RecursiveVisitor { - final TestMetadataRepository repository; +/// Visit the given component assigning [Metadata] object created with +/// [Metadata.forNode] to each supported node in the component which matches +/// [shouldAnnotate] predicate. +void annotate(Component p, NodePredicate shouldAnnotate) { + globalDebuggingNames = new NameSystem(); + final repository = p.metadata[TestMetadataRepository.kTag]; + p.accept(new Visitor(shouldAnnotate, (node) { + repository.mapping[node] = new Metadata.forNode(node); + })); +} - Validator(Component component) - : repository = component.metadata[TestMetadataRepository.kTag]; +/// Visit the given component and checks that each supported node in the +/// component matching [shouldAnnotate] predicate has correct metadata. +void validate(Component p, NodePredicate shouldAnnotate) { + globalDebuggingNames = new NameSystem(); + final repository = p.metadata[TestMetadataRepository.kTag]; + p.accept(new Visitor(shouldAnnotate, (node) { + final m = repository.mapping[node]; + final expected = new Metadata.forNode(node); - defaultTreeNode(TreeNode node) { - super.defaultTreeNode(node); - if (MetadataRepository.isSupported(node)) { - final m = repository.mapping[node]; - final expected = new Metadata.forNode(node); - - expect(m.string, equals(expected.string)); - expect(m.member, equals(expected.member)); - expect(m.type, equals(expected.type)); - } - } - - static void validate(Component p) { - globalDebuggingNames = new NameSystem(); - p.accept(new Validator(p)); - } + expect(m, isNotNull); + expect(m.string, equals(expected.string)); + expect(m.member, equals(expected.member)); + expect(m.type, equals(expected.type)); + })); } Component fromBinary(List bytes) { var component = new Component(); component.addMetadataRepository(new TestMetadataRepository()); - new BinaryBuilderWithMetadata(bytes).readSingleFileComponent(component); + new BinaryBuilderWithMetadata(bytes).readComponent(component); return component; } @@ -147,19 +145,53 @@ List toBinary(Component p) { return sink.builder.takeBytes(); } -main() { - test('annotate-serialize-deserialize-validate', () async { - final Uri platform = computePlatformBinariesLocation(forceBuildDir: true) - .resolve("vm_platform_strong.dill"); - final List platformBinary = - await new File(platform.toFilePath()).readAsBytes(); +main() async { + bool anyNode(TreeNode node) => true; + bool onlyMethods(TreeNode node) => + node is Procedure && + node.kind == ProcedureKind.Method && + node.enclosingClass != null; + final Uri platform = computePlatformBinariesLocation(forceBuildDir: true) + .resolve("vm_platform_strong.dill"); + final List platformBinary = + await new File(platform.toFilePath()).readAsBytes(); + + Future testRoundTrip(List Function(List) binaryTransformer, + NodePredicate shouldAnnotate) async { final component = fromBinary(platformBinary); - Annotator.annotate(component); - Validator.validate(component); + annotate(component, shouldAnnotate); + validate(component, shouldAnnotate); + expect(component.metadata[TestMetadataRepository.kTag].mapping.length, + greaterThan(0)); - final annotatedComponentBinary = toBinary(component); + final annotatedComponentBinary = binaryTransformer(toBinary(component)); final annotatedComponentFromBinary = fromBinary(annotatedComponentBinary); - Validator.validate(annotatedComponentFromBinary); + validate(annotatedComponentFromBinary, shouldAnnotate); + expect( + annotatedComponentFromBinary + .metadata[TestMetadataRepository.kTag].mapping.length, + greaterThan(0)); + } + + test('annotate-serialize-deserialize-validate', () async { + await testRoundTrip((binary) => binary, anyNode); + }); + + test('annotate-serialize-deserialize-validate-only-methods', () async { + await testRoundTrip((binary) => binary, onlyMethods); + }); + + test('annotate-serialize-deserialize-twice-then-validate', () async { + // This test validates that serializing a component that was just + // deserialized (without visiting anything) works. + await testRoundTrip((binary) => toBinary(fromBinary(binary)), anyNode); + }); + + test('annotate-serialize-deserialize-twice-then-validate-only-methods', + () async { + // This test validates that serializing a component that was just + // deserialized (without visiting anything) works. + await testRoundTrip((binary) => toBinary(fromBinary(binary)), onlyMethods); }); }