From 927c59104f2a6519a6029c3fc84f87f80c616cdb Mon Sep 17 00:00:00 2001 From: Tess Strickland Date: Thu, 22 Apr 2021 15:27:38 +0000 Subject: [PATCH] [vm] Re-tighten requirements that the node type be set before writing. Fixes the case where the Image name and type was not written to the v8 snapshot profile in non-precompiled mode. Adds core-jit snapshot tests to the v8 snapshot profile writer test. In core-jit and app-aot-elf modes, verify that all bytes in the snapshot sections are accounted for in the profile. Bug: https://github.com/dart-lang/sdk/issues/45787 TEST=vm/dart{,_2}/v8_snapshot_profile_writer_test Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-product-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-kernel-precomp-linux-debug-x64-try Change-Id: I88be309cdea6293d768e2fa458492387d70fde22 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196323 Commit-Queue: Tess Strickland Reviewed-by: Martin Kustermann Reviewed-by: Daco Harkes --- pkg/native_stack_traces/CHANGELOG.md | 4 + pkg/native_stack_traces/lib/elf.dart | 11 + .../lib/src/constants.dart | 6 + pkg/native_stack_traces/pubspec.yaml | 2 +- .../tests/vm/dart/use_flag_test_helper.dart | 9 +- .../dart/v8_snapshot_profile_writer_test.dart | 315 ++++++++++++------ .../tests/vm/dart_2/use_flag_test_helper.dart | 9 +- .../v8_snapshot_profile_writer_test.dart | 299 +++++++++++------ runtime/vm/image_snapshot.cc | 14 +- runtime/vm/v8_snapshot_writer.cc | 10 +- runtime/vm/v8_snapshot_writer.h | 1 - 11 files changed, 454 insertions(+), 226 deletions(-) create mode 100644 pkg/native_stack_traces/lib/elf.dart diff --git a/pkg/native_stack_traces/CHANGELOG.md b/pkg/native_stack_traces/CHANGELOG.md index 9b16d1c5503..4a9f87382c5 100644 --- a/pkg/native_stack_traces/CHANGELOG.md +++ b/pkg/native_stack_traces/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 0.4.1 + +- Exported some ELF utilities in lib/elf.dart for use in Dart tests. + ## 0.4.0 - Stable null safe version of package. diff --git a/pkg/native_stack_traces/lib/elf.dart b/pkg/native_stack_traces/lib/elf.dart new file mode 100644 index 00000000000..de32cd3b4f9 --- /dev/null +++ b/pkg/native_stack_traces/lib/elf.dart @@ -0,0 +1,11 @@ +// 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. + +export 'src/elf.dart' show Elf, Symbol; +export 'src/constants.dart' + show + isolateDataSymbolName, + isolateSymbolName, + vmDataSymbolName, + vmSymbolName; diff --git a/pkg/native_stack_traces/lib/src/constants.dart b/pkg/native_stack_traces/lib/src/constants.dart index c44d615d845..ca987f57ba7 100644 --- a/pkg/native_stack_traces/lib/src/constants.dart +++ b/pkg/native_stack_traces/lib/src/constants.dart @@ -12,5 +12,11 @@ const String buildIdNoteName = "GNU"; // The dynamic symbol name for the VM instructions section. const String vmSymbolName = "_kDartVmSnapshotInstructions"; +// The dynamic symbol name for the VM data section. +const String vmDataSymbolName = "_kDartVmSnapshotData"; + // The dynamic symbol name for the isolate instructions section. const String isolateSymbolName = "_kDartIsolateSnapshotInstructions"; + +// The dynamic symbol name for the isolate data section. +const String isolateDataSymbolName = "_kDartIsolateSnapshotData"; diff --git a/pkg/native_stack_traces/pubspec.yaml b/pkg/native_stack_traces/pubspec.yaml index df71c5ae66a..f93e01b9d21 100644 --- a/pkg/native_stack_traces/pubspec.yaml +++ b/pkg/native_stack_traces/pubspec.yaml @@ -1,6 +1,6 @@ name: native_stack_traces description: Utilities for working with non-symbolic stack traces. -version: 0.4.0 +version: 0.4.1 homepage: https://github.com/dart-lang/sdk/tree/master/pkg/native_stack_traces diff --git a/runtime/tests/vm/dart/use_flag_test_helper.dart b/runtime/tests/vm/dart/use_flag_test_helper.dart index d6854aca139..431fce51195 100644 --- a/runtime/tests/vm/dart/use_flag_test_helper.dart +++ b/runtime/tests/vm/dart/use_flag_test_helper.dart @@ -116,13 +116,16 @@ Future runHelper(String executable, List args) async { print('Running $executable ${args.join(' ')}'); final result = await Process.run(executable, args); + print('Subcommand terminated with exit code ${result.exitCode}.'); if (result.stdout.isNotEmpty) { print('Subcommand stdout:'); print(result.stdout); } - if (result.stderr.isNotEmpty) { - print('Subcommand stderr:'); - print(result.stderr); + if (result.exitCode != 0) { + if (result.stderr.isNotEmpty) { + print('Subcommand stderr:'); + print(result.stderr); + } } return result; diff --git a/runtime/tests/vm/dart/v8_snapshot_profile_writer_test.dart b/runtime/tests/vm/dart/v8_snapshot_profile_writer_test.dart index aa7eac0787f..8719b765991 100644 --- a/runtime/tests/vm/dart/v8_snapshot_profile_writer_test.dart +++ b/runtime/tests/vm/dart/v8_snapshot_profile_writer_test.dart @@ -6,6 +6,7 @@ import 'dart:convert'; import 'dart:io'; import 'package:expect/expect.dart'; +import 'package:native_stack_traces/elf.dart'; import 'package:path/path.dart' as path; import 'package:vm_snapshot_analysis/v8_profile.dart'; @@ -14,33 +15,112 @@ import 'use_flag_test_helper.dart'; // Used to ensure we don't have multiple equivalent calls to test. final _seenDescriptions = {}; -Future test(String dillPath, +Snapshot testProfile(String profilePath) { + final profile = + Snapshot.fromJson(jsonDecode(File(profilePath).readAsStringSync())); + + // Verify that there are no "unknown" nodes. These are emitted when we see a + // reference to an some object but no other metadata about the object was + // recorded. We should at least record the type for every object in the + // graph (in some cases the shallow size can legitimately be 0, e.g. for + // "base objects" not written to the snapshot or artificial nodes). + for (final node in profile.nodes) { + Expect.notEquals("Unknown", node.type, "unknown node ${node}"); + } + + final root = profile.nodeAt(0); + final reachable = {}; + + // HeapSnapshotWorker.HeapSnapshot.calculateDistances (from HeapSnapshot.js) + // assumes that the graph root has at most one edge to any other node + // (most likely an oversight). + for (final edge in root.edges) { + Expect.isTrue( + reachable.add(edge.target), + "root\n\n$root\n\nhas multiple edges to node\n\n${edge.target}:\n\n" + "${root.edges.where((e) => e.target == edge.target).toList()}"); + } + + // Check that all other nodes are reachable from the root. + final stack = [...reachable]; + while (!stack.isEmpty) { + final next = stack.removeLast(); + for (final edge in next.edges) { + if (reachable.add(edge.target)) { + stack.add(edge.target); + } + } + } + + final unreachable = + profile.nodes.skip(1).where((Node n) => !reachable.contains(n)).toSet(); + Expect.isEmpty(unreachable); + + return profile; +} + +Future testJIT(String dillPath) async { + final description = 'jit'; + Expect.isTrue(_seenDescriptions.add(description), + "test configuration $description would be run multiple times"); + + await withTempDir('v8-snapshot-profile-$description', (String tempDir) async { + // Generate the snapshot profile. + final profilePath = path.join(tempDir, 'profile.heapsnapshot'); + final vmTextPath = path.join(tempDir, 'vm_instructions.bin'); + final isolateTextPath = path.join(tempDir, 'isolate_instructions.bin'); + final vmDataPath = path.join(tempDir, 'vm_data.bin'); + final isolateDataPath = path.join(tempDir, 'isolate_data.bin'); + + await run(genSnapshot, [ + '--snapshot-kind=core-jit', + '--vm_snapshot_instructions=$vmTextPath', + '--isolate_snapshot_instructions=$isolateTextPath', + '--vm_snapshot_data=$vmDataPath', + '--isolate_snapshot_data=$isolateDataPath', + "--write-v8-snapshot-profile-to=$profilePath", + dillPath, + ]); + + print("Snapshot profile generated at $profilePath."); + + final profile = testProfile(profilePath); + + // Verify that the total size of the snapshot text and data sections is + // the same as the sum of the shallow sizes of all objects in the profile. + // This ensures that all bytes are accounted for in some way. + final actualSize = await File(vmTextPath).length() + + await File(isolateTextPath).length() + + await File(vmDataPath).length() + + await File(isolateDataPath).length(); + final expectedSize = + profile.nodes.fold(0, (size, n) => size + n.selfSize); + + Expect.equals(expectedSize, actualSize, "failed on $description snapshot"); + }); +} + +Future testAOT(String dillPath, {bool useAsm = false, bool useBare = true, bool forceDrops = false, bool useDispatch = true, - bool stripUtil = false, // Note: forced if useAsm. - bool stripFlag = false, // Note: forced if !stripUtil (and thus !useAsm). + bool stripUtil = false, // Note: forced true if useAsm. + bool stripFlag = false, bool disassemble = false}) async { - // We don't assume forced disassembler support in Product mode, so skip any - // disassembly test. - if (!const bool.fromEnvironment('dart.vm.product') && disassemble) { - return; + if (const bool.fromEnvironment('dart.vm.product') && disassemble) { + Expect.isFalse(disassemble, 'no use of disassembler in PRODUCT mode'); } - // The assembler may add extra unnecessary information to the compiled - // snapshot whether or not we generate DWARF information in the assembly, so - // we force the use of a utility when generating assembly. + // For assembly, we can't test the sizes of the snapshot sections, since we + // don't have a Mach-O reader for Mac snapshots and for ELF, the assembler + // merges the text/data sections and the VM/isolate section symbols may not + // have length information. Thus, we force external stripping so we can test + // the approximate size of the stripped snapshot. if (useAsm) { stripUtil = true; } - // We must strip the output in some way when generating ELF snapshots, - // else the debugging information added will cause the test to fail. - if (!stripUtil) { - stripFlag = true; - } - final descriptionBuilder = StringBuffer()..write(useAsm ? 'assembly' : 'elf'); if (!useBare) { descriptionBuilder.write('-nonbare'); @@ -70,7 +150,7 @@ Future test(String dillPath, final profilePath = path.join(tempDir, 'profile.heapsnapshot'); final snapshotPath = path.join(tempDir, 'test.snap'); final commonSnapshotArgs = [ - if (stripFlag) '--strip', + if (stripFlag) '--strip', // gen_snapshot specific and not a VM flag. useBare ? '--use-bare-instructions' : '--no-use-bare-instructions', "--write-v8-snapshot-profile-to=$profilePath", if (forceDrops) ...[ @@ -78,9 +158,8 @@ Future test(String dillPath, '--no-retain-function-objects', '--no-retain-code-objects' ], - if (!useDispatch) '--no-use-table-dispatch', - if (disassemble) '--disassemble', - '--ignore-unrecognized-flags', + useDispatch ? '--use-table-dispatch' : '--no-use-table-dispatch', + if (disassemble) '--disassemble', // Not defined in PRODUCT mode. dillPath, ]; @@ -102,70 +181,70 @@ Future test(String dillPath, ]); } - String strippedPath; - if (stripUtil) { - strippedPath = snapshotPath + '.stripped'; - await stripSnapshot(snapshotPath, strippedPath, forceElf: !useAsm); - } else { - strippedPath = snapshotPath; - } - + print("Snapshot generated at $snapshotPath."); print("Snapshot profile generated at $profilePath."); - final profile = - Snapshot.fromJson(jsonDecode(File(profilePath).readAsStringSync())); + final profile = testProfile(profilePath); - // Verify that there are no "unknown" nodes. These are emitted when we see a - // reference to an some object but no other metadata about the object was - // recorded. We should at least record the type for every object in the - // graph (in some cases the shallow size can legitimately be 0, e.g. for - // "base objects" not written to the snapshot or artificial nodes). - for (final node in profile.nodes) { - Expect.notEquals("Unknown", node.type, "unknown node ${node}"); + final expectedSize = + profile.nodes.fold(0, (size, n) => size + n.selfSize); + + var checkedSize = false; + if (!useAsm) { + // Verify that the total size of the snapshot text and data sections is + // the same as the sum of the shallow sizes of all objects in the profile. + // This ensures that all bytes are accounted for in some way. + final elf = Elf.fromFile(snapshotPath); + Expect.isNotNull(elf); + elf!; // To refine type to non-nullable version. + + final vmTextSectionSymbol = elf.dynamicSymbolFor(vmSymbolName); + Expect.isNotNull(vmTextSectionSymbol); + final vmDataSectionSymbol = elf.dynamicSymbolFor(vmDataSymbolName); + Expect.isNotNull(vmDataSectionSymbol); + final isolateTextSectionSymbol = elf.dynamicSymbolFor(isolateSymbolName); + Expect.isNotNull(isolateTextSectionSymbol); + final isolateDataSectionSymbol = + elf.dynamicSymbolFor(isolateDataSymbolName); + Expect.isNotNull(isolateDataSectionSymbol); + + final actualSize = vmTextSectionSymbol!.size + + vmDataSectionSymbol!.size + + isolateTextSectionSymbol!.size + + isolateDataSectionSymbol!.size; + + Expect.equals( + expectedSize, actualSize, "failed on $description snapshot"); + checkedSize = true; } - final root = profile.nodeAt(0); - final reachable = {}; - - // HeapSnapshotWorker.HeapSnapshot.calculateDistances (from HeapSnapshot.js) - // assumes that the graph root has at most one edge to any other node - // (most likely an oversight). - for (final edge in root.edges) { - Expect.isTrue( - reachable.add(edge.target), - "root\n\n$root\n\nhas multiple edges to node\n\n${edge.target}:\n\n" - "${root.edges.where((e) => e.target == edge.target).toList()}"); - } - - // Check that all other nodes are reachable from the root. - final stack = [...reachable]; - while (!stack.isEmpty) { - final next = stack.removeLast(); - for (final edge in next.edges) { - if (reachable.add(edge.target)) { - stack.add(edge.target); - } + if (stripUtil || stripFlag) { + var strippedSnapshotPath = snapshotPath; + if (stripUtil) { + strippedSnapshotPath = snapshotPath + '.stripped'; + await stripSnapshot(snapshotPath, strippedSnapshotPath, + forceElf: !useAsm); + print("Stripped snapshot generated at $strippedSnapshotPath."); } + + // Verify that the actual size of the stripped snapshot is close to the + // sum of the shallow sizes of all objects in the profile. They will not + // be exactly equal because of global headers and padding. + final actualSize = await File(strippedSnapshotPath).length(); + + // See Elf::kPages in runtime/vm/elf.h, which is also used for assembly + // padding. + final segmentAlignment = 16 * 1024; + // Not every byte is accounted for by the snapshot profile, and data and + // instruction segments are padded to an alignment boundary. + final tolerance = 0.03 * actualSize + 2 * segmentAlignment; + + Expect.approxEquals(expectedSize, actualSize, tolerance, + "failed on $description snapshot"); + checkedSize = true; } - final unreachable = - profile.nodes.skip(1).where((Node n) => !reachable.contains(n)).toSet(); - Expect.isEmpty(unreachable); - - // Verify that the actual size of the snapshot is close to the sum of the - // shallow sizes of all objects in the profile. They will not be exactly - // equal because of global headers and padding. - final actual = await File(strippedPath).length(); - final expected = profile.nodes.fold(0, (size, n) => size + n.selfSize); - - // See Elf::kPages in runtime/vm/elf.h. - final segmentAlignment = 16 * 1024; - // Not every byte is accounted for by the snapshot profile, and data and - // instruction segments are padded to an alignment boundary. - final tolerance = 0.03 * actual + 2 * segmentAlignment; - - Expect.approxEquals( - expected, actual, tolerance, "failed on $description snapshot"); + Expect.isTrue(checkedSize, "no snapshot size checks were performed"); }); } @@ -264,10 +343,24 @@ main() async { await testMacros(); await withTempDir('v8-snapshot-profile-writer', (String tempDir) async { - // We only need to generate the dill file once. + // We only need to generate the dill file once for all JIT tests. final _thisTestPath = path.join(sdkDir, 'runtime', 'tests', 'vm', 'dart', 'v8_snapshot_profile_writer_test.dart'); - final dillPath = path.join(tempDir, 'test.dill'); + final jitDillPath = path.join(tempDir, 'jit_test.dill'); + await run(genKernel, [ + '--platform', + platformDill, + ...Platform.executableArguments.where((arg) => + arg.startsWith('--enable-experiment=') || + arg == '--sound-null-safety' || + arg == '--no-sound-null-safety'), + '-o', + jitDillPath, + _thisTestPath + ]); + + // We only need to generate the dill file once for all AOT tests. + final aotDillPath = path.join(tempDir, 'aot_test.dill'); await run(genKernel, [ '--aot', '--platform', @@ -277,26 +370,38 @@ main() async { arg == '--sound-null-safety' || arg == '--no-sound-null-safety'), '-o', - dillPath, + aotDillPath, _thisTestPath ]); - // Just as a reminder (these rules are applied in order inside test): - // If useAsm is true, then stripUtil is forced (as the assembler may add - // extra information that needs stripping). - // If stripUtil is false, then stripFlag is forced (as the output must be - // stripped in some way to remove DWARF information). + // Just as a reminder for AOT tests: + // * If useAsm is true, then stripUtil is forced (as the assembler may add + // extra information that needs stripping), so no need to specify + // stripUtil for useAsm tests. - // Test stripped ELF generation directly. - await test(dillPath); - await test(dillPath, useBare: false); - await test(dillPath, forceDrops: true); - await test(dillPath, forceDrops: true, useBare: false); - await test(dillPath, forceDrops: true, useDispatch: false); - await test(dillPath, forceDrops: true, useDispatch: false, useBare: false); + // Test profile generation with a core JIT snapshot. + await testJIT(jitDillPath); - // Regression test for dartbug.com/41149. - await test(dillPath, useBare: false, disassemble: true); + // Test unstripped ELF generation directly. + await testAOT(aotDillPath); + await testAOT(aotDillPath, useBare: false); + await testAOT(aotDillPath, forceDrops: true); + await testAOT(aotDillPath, forceDrops: true, useBare: false); + await testAOT(aotDillPath, forceDrops: true, useDispatch: false); + await testAOT(aotDillPath, + forceDrops: true, useDispatch: false, useBare: false); + + // Test flag-stripped ELF generation. + await testAOT(aotDillPath, stripFlag: true); + await testAOT(aotDillPath, useBare: false, stripFlag: true); + + // Since we can't force disassembler support after the fact when running + // in PRODUCT mode, skip any --disassemble tests. Do these tests last as + // they have lots of output and so the log will be truncated. + if (!const bool.fromEnvironment('dart.vm.product')) { + // Regression test for dartbug.com/41149. + await testAOT(aotDillPath, useBare: false, disassemble: true); + } // We neither generate assembly nor have a stripping utility on Windows. if (Platform.isWindows) { @@ -308,24 +413,24 @@ main() async { if (Platform.isMacOS && clangBuildToolsDir == null) { printSkip('ELF external stripping test'); } else { - // Test unstripped ELF generation that is then stripped externally. - await test(dillPath, stripUtil: true); - await test(dillPath, stripUtil: true, useBare: false); + // Test unstripped ELF generation that is then externally stripped. + await testAOT(aotDillPath, stripUtil: true); + await testAOT(aotDillPath, stripUtil: true, useBare: false); } // TODO(sstrickl): Currently we can't assemble for SIMARM64 on MacOSX. - // For example, the test runner still uses blobs for dartkp-mac-*-simarm64. - // Change assembleSnapshot and remove this check when we can. + // For example, the test runner still uses blobs for + // dartkp-mac-*-simarm64. Change assembleSnapshot and remove this check + // when we can. if (Platform.isMacOS && buildDir.endsWith('SIMARM64')) { printSkip('assembly tests'); return; } - - // Test unstripped assembly generation that is then compiled and stripped. - await test(dillPath, useAsm: true); - await test(dillPath, useAsm: true, useBare: false); - // Test stripped assembly generation that is then compiled and stripped. - await test(dillPath, useAsm: true, stripFlag: true); - await test(dillPath, useAsm: true, stripFlag: true, useBare: false); + // Test unstripped assembly generation that is then externally stripped. + await testAOT(aotDillPath, useAsm: true); + await testAOT(aotDillPath, useAsm: true, useBare: false); + // Test stripped assembly generation that is then externally stripped. + await testAOT(aotDillPath, useAsm: true, stripFlag: true); + await testAOT(aotDillPath, useAsm: true, stripFlag: true, useBare: false); }); } diff --git a/runtime/tests/vm/dart_2/use_flag_test_helper.dart b/runtime/tests/vm/dart_2/use_flag_test_helper.dart index 24f0687ab73..650a9bf509b 100644 --- a/runtime/tests/vm/dart_2/use_flag_test_helper.dart +++ b/runtime/tests/vm/dart_2/use_flag_test_helper.dart @@ -114,13 +114,16 @@ Future runHelper(String executable, List args) async { print('Running $executable ${args.join(' ')}'); final result = await Process.run(executable, args); + print('Subcommand terminated with exit code ${result.exitCode}.'); if (result.stdout.isNotEmpty) { print('Subcommand stdout:'); print(result.stdout); } - if (result.stderr.isNotEmpty) { - print('Subcommand stderr:'); - print(result.stderr); + if (result.exitCode != 0) { + if (result.stderr.isNotEmpty) { + print('Subcommand stderr:'); + print(result.stderr); + } } return result; diff --git a/runtime/tests/vm/dart_2/v8_snapshot_profile_writer_test.dart b/runtime/tests/vm/dart_2/v8_snapshot_profile_writer_test.dart index 7977a710850..0628aa0b6fc 100644 --- a/runtime/tests/vm/dart_2/v8_snapshot_profile_writer_test.dart +++ b/runtime/tests/vm/dart_2/v8_snapshot_profile_writer_test.dart @@ -6,6 +6,7 @@ import 'dart:convert'; import 'dart:io'; import 'package:expect/expect.dart'; +import 'package:native_stack_traces/elf.dart'; import 'package:path/path.dart' as path; import 'package:vm_snapshot_analysis/v8_profile.dart'; @@ -14,33 +15,112 @@ import 'use_flag_test_helper.dart'; // Used to ensure we don't have multiple equivalent calls to test. final _seenDescriptions = {}; -Future test(String dillPath, +Snapshot testProfile(String profilePath) { + final profile = + Snapshot.fromJson(jsonDecode(File(profilePath).readAsStringSync())); + + // Verify that there are no "unknown" nodes. These are emitted when we see a + // reference to an some object but no other metadata about the object was + // recorded. We should at least record the type for every object in the + // graph (in some cases the shallow size can legitimately be 0, e.g. for + // "base objects" not written to the snapshot or artificial nodes). + for (final node in profile.nodes) { + Expect.notEquals("Unknown", node.type, "unknown node ${node}"); + } + + final root = profile.nodeAt(0); + final reachable = {}; + + // HeapSnapshotWorker.HeapSnapshot.calculateDistances (from HeapSnapshot.js) + // assumes that the graph root has at most one edge to any other node + // (most likely an oversight). + for (final edge in root.edges) { + Expect.isTrue( + reachable.add(edge.target), + "root\n\n$root\n\nhas multiple edges to node\n\n${edge.target}:\n\n" + "${root.edges.where((e) => e.target == edge.target).toList()}"); + } + + // Check that all other nodes are reachable from the root. + final stack = [...reachable]; + while (!stack.isEmpty) { + final next = stack.removeLast(); + for (final edge in next.edges) { + if (reachable.add(edge.target)) { + stack.add(edge.target); + } + } + } + + final unreachable = + profile.nodes.skip(1).where((Node n) => !reachable.contains(n)).toSet(); + Expect.isEmpty(unreachable); + + return profile; +} + +Future testJIT(String dillPath) async { + final description = 'jit'; + Expect.isTrue(_seenDescriptions.add(description), + "test configuration $description would be run multiple times"); + + await withTempDir('v8-snapshot-profile-$description', (String tempDir) async { + // Generate the snapshot profile. + final profilePath = path.join(tempDir, 'profile.heapsnapshot'); + final vmTextPath = path.join(tempDir, 'vm_instructions.bin'); + final isolateTextPath = path.join(tempDir, 'isolate_instructions.bin'); + final vmDataPath = path.join(tempDir, 'vm_data.bin'); + final isolateDataPath = path.join(tempDir, 'isolate_data.bin'); + + await run(genSnapshot, [ + '--snapshot-kind=core-jit', + '--vm_snapshot_instructions=$vmTextPath', + '--isolate_snapshot_instructions=$isolateTextPath', + '--vm_snapshot_data=$vmDataPath', + '--isolate_snapshot_data=$isolateDataPath', + "--write-v8-snapshot-profile-to=$profilePath", + dillPath, + ]); + + print("Snapshot profile generated at $profilePath."); + + final profile = testProfile(profilePath); + + // Verify that the total size of the snapshot text and data sections is + // the same as the sum of the shallow sizes of all objects in the profile. + // This ensures that all bytes are accounted for in some way. + final actualSize = await File(vmTextPath).length() + + await File(isolateTextPath).length() + + await File(vmDataPath).length() + + await File(isolateDataPath).length(); + final expectedSize = + profile.nodes.fold(0, (size, n) => size + n.selfSize); + + Expect.equals(expectedSize, actualSize, "failed on $description snapshot"); + }); +} + +Future testAOT(String dillPath, {bool useAsm = false, bool useBare = true, bool forceDrops = false, bool useDispatch = true, - bool stripUtil = false, // Note: forced if useAsm. - bool stripFlag = false, // Note: forced if !stripUtil (and thus !useAsm). + bool stripUtil = false, // Note: forced true if useAsm. + bool stripFlag = false, bool disassemble = false}) async { - // We don't assume forced disassembler support in Product mode, so skip any - // disassembly test. - if (!const bool.fromEnvironment('dart.vm.product') && disassemble) { - return; + if (const bool.fromEnvironment('dart.vm.product') && disassemble) { + Expect.isFalse(disassemble, 'no use of disassembler in PRODUCT mode'); } - // The assembler may add extra unnecessary information to the compiled - // snapshot whether or not we generate DWARF information in the assembly, so - // we force the use of a utility when generating assembly. + // For assembly, we can't test the sizes of the snapshot sections, since we + // don't have a Mach-O reader for Mac snapshots and for ELF, the assembler + // merges the text/data sections and the VM/isolate section symbols may not + // have length information. Thus, we force external stripping so we can test + // the approximate size of the stripped snapshot. if (useAsm) { stripUtil = true; } - // We must strip the output in some way when generating ELF snapshots, - // else the debugging information added will cause the test to fail. - if (!stripUtil) { - stripFlag = true; - } - final descriptionBuilder = StringBuffer()..write(useAsm ? 'assembly' : 'elf'); if (!useBare) { descriptionBuilder.write('-nonbare'); @@ -70,7 +150,7 @@ Future test(String dillPath, final profilePath = path.join(tempDir, 'profile.heapsnapshot'); final snapshotPath = path.join(tempDir, 'test.snap'); final commonSnapshotArgs = [ - if (stripFlag) '--strip', + if (stripFlag) '--strip', // gen_snapshot specific and not a VM flag. useBare ? '--use-bare-instructions' : '--no-use-bare-instructions', "--write-v8-snapshot-profile-to=$profilePath", if (forceDrops) ...[ @@ -78,9 +158,8 @@ Future test(String dillPath, '--no-retain-function-objects', '--no-retain-code-objects' ], - if (!useDispatch) '--no-use-table-dispatch', - if (disassemble) '--disassemble', - '--ignore-unrecognized-flags', + useDispatch ? '--use-table-dispatch' : '--no-use-table-dispatch', + if (disassemble) '--disassemble', // Not defined in PRODUCT mode. dillPath, ]; @@ -110,62 +189,69 @@ Future test(String dillPath, strippedPath = snapshotPath; } + print("Snapshot generated at $snapshotPath."); print("Snapshot profile generated at $profilePath."); - final profile = - Snapshot.fromJson(jsonDecode(File(profilePath).readAsStringSync())); + final profile = testProfile(profilePath); - // Verify that there are no "unknown" nodes. These are emitted when we see a - // reference to an some object but no other metadata about the object was - // recorded. We should at least record the type for every object in the - // graph (in some cases the shallow size can legitimately be 0, e.g. for - // "base objects" not written to the snapshot or artificial nodes). - for (final node in profile.nodes) { - Expect.notEquals("Unknown", node.type, "unknown node ${node}"); + final expectedSize = + profile.nodes.fold(0, (size, n) => size + n.selfSize); + + var checkedSize = false; + if (!useAsm) { + // Verify that the total size of the snapshot text and data sections is + // the same as the sum of the shallow sizes of all objects in the profile. + // This ensures that all bytes are accounted for in some way. + final elf = Elf.fromFile(snapshotPath); + Expect.isNotNull(elf); + + final vmTextSectionSymbol = elf.dynamicSymbolFor(vmSymbolName); + Expect.isNotNull(vmTextSectionSymbol); + final vmDataSectionSymbol = elf.dynamicSymbolFor(vmDataSymbolName); + Expect.isNotNull(vmDataSectionSymbol); + final isolateTextSectionSymbol = elf.dynamicSymbolFor(isolateSymbolName); + Expect.isNotNull(isolateTextSectionSymbol); + final isolateDataSectionSymbol = + elf.dynamicSymbolFor(isolateDataSymbolName); + Expect.isNotNull(isolateDataSectionSymbol); + + final actualSize = vmTextSectionSymbol.size + + vmDataSectionSymbol.size + + isolateTextSectionSymbol.size + + isolateDataSectionSymbol.size; + + Expect.equals( + expectedSize, actualSize, "failed on $description snapshot"); + checkedSize = true; } - final root = profile.nodeAt(0); - final reachable = {}; - - // HeapSnapshotWorker.HeapSnapshot.calculateDistances (from HeapSnapshot.js) - // assumes that the graph root has at most one edge to any other node - // (most likely an oversight). - for (final edge in root.edges) { - Expect.isTrue( - reachable.add(edge.target), - "root\n\n$root\n\nhas multiple edges to node\n\n${edge.target}:\n\n" - "${root.edges.where((e) => e.target == edge.target).toList()}"); - } - - // Check that all other nodes are reachable from the root. - final stack = [...reachable]; - while (!stack.isEmpty) { - final next = stack.removeLast(); - for (final edge in next.edges) { - if (reachable.add(edge.target)) { - stack.add(edge.target); - } + if (stripUtil || stripFlag) { + var strippedSnapshotPath = snapshotPath; + if (stripUtil) { + strippedSnapshotPath = snapshotPath + '.stripped'; + await stripSnapshot(snapshotPath, strippedSnapshotPath, + forceElf: !useAsm); + print("Stripped snapshot generated at $strippedSnapshotPath."); } + + // Verify that the actual size of the stripped snapshot is close to the + // sum of the shallow sizes of all objects in the profile. They will not + // be exactly equal because of global headers and padding. + final actualSize = await File(strippedSnapshotPath).length(); + + // See Elf::kPages in runtime/vm/elf.h, which is also used for assembly + // padding. + final segmentAlignment = 16 * 1024; + // Not every byte is accounted for by the snapshot profile, and data and + // instruction segments are padded to an alignment boundary. + final tolerance = 0.03 * actualSize + 2 * segmentAlignment; + + Expect.approxEquals(expectedSize, actualSize, tolerance, + "failed on $description snapshot"); + checkedSize = true; } - final unreachable = - profile.nodes.skip(1).where((Node n) => !reachable.contains(n)).toSet(); - Expect.isEmpty(unreachable); - - // Verify that the actual size of the snapshot is close to the sum of the - // shallow sizes of all objects in the profile. They will not be exactly - // equal because of global headers and padding. - final actual = await File(strippedPath).length(); - final expected = profile.nodes.fold(0, (size, n) => size + n.selfSize); - - // See Elf::kPages in runtime/vm/elf.h. - final segmentAlignment = 16 * 1024; - // Not every byte is accounted for by the snapshot profile, and data and - // instruction segments are padded to an alignment boundary. - final tolerance = 0.03 * actual + 2 * segmentAlignment; - - Expect.approxEquals( - expected, actual, tolerance, "failed on $description snapshot"); + Expect.isTrue(checkedSize, "no snapshot size checks were performed"); }); } @@ -264,35 +350,52 @@ main() async { await testMacros(); await withTempDir('v8-snapshot-profile-writer', (String tempDir) async { - // We only need to generate the dill file once. - final _thisTestPath = path.join(sdkDir, 'runtime', 'tests', 'vm', 'dart_2', + // We only need to generate the dill file once for all JIT tests. + final _thisTestPath = path.join(sdkDir, 'runtime', 'tests', 'vm', 'dart', 'v8_snapshot_profile_writer_test.dart'); - final dillPath = path.join(tempDir, 'test.dill'); + final jitDillPath = path.join(tempDir, 'jit_test.dill'); + await run(genKernel, + ['--platform', platformDill, '-o', jitDillPath, _thisTestPath]); + + // We only need to generate the dill file once for all AOT tests. + final aotDillPath = path.join(tempDir, 'aot_test.dill'); await run(genKernel, [ '--aot', '--platform', platformDill, '-o', - dillPath, + aotDillPath, _thisTestPath ]); - // Just as a reminder (these rules are applied in order inside test): - // If useAsm is true, then stripUtil is forced (as the assembler may add - // extra information that needs stripping). - // If stripUtil is false, then stripFlag is forced (as the output must be - // stripped in some way to remove DWARF information). + // Just as a reminder for AOT tests: + // * If useAsm is true, then stripUtil is forced (as the assembler may add + // extra information that needs stripping), so no need to specify + // stripUtil for useAsm tests. - // Test stripped ELF generation directly. - await test(dillPath); - await test(dillPath, useBare: false); - await test(dillPath, forceDrops: true); - await test(dillPath, forceDrops: true, useBare: false); - await test(dillPath, forceDrops: true, useDispatch: false); - await test(dillPath, forceDrops: true, useDispatch: false, useBare: false); + // Test profile generation with a core JIT snapshot. + await testJIT(jitDillPath); - // Regression test for dartbug.com/41149. - await test(dillPath, useBare: false, disassemble: true); + // Test unstripped ELF generation directly. + await testAOT(aotDillPath); + await testAOT(aotDillPath, useBare: false); + await testAOT(aotDillPath, forceDrops: true); + await testAOT(aotDillPath, forceDrops: true, useBare: false); + await testAOT(aotDillPath, forceDrops: true, useDispatch: false); + await testAOT(aotDillPath, + forceDrops: true, useDispatch: false, useBare: false); + + // Test flag-stripped ELF generation. + await testAOT(aotDillPath, stripFlag: true); + await testAOT(aotDillPath, useBare: false, stripFlag: true); + + // Since we can't force disassembler support after the fact when running + // in PRODUCT mode, skip any --disassemble tests. Do these tests last as + // they have lots of output and so the log will be truncated. + if (!const bool.fromEnvironment('dart.vm.product')) { + // Regression test for dartbug.com/41149. + await testAOT(aotDillPath, useBare: false, disassemble: true); + } // We neither generate assembly nor have a stripping utility on Windows. if (Platform.isWindows) { @@ -304,24 +407,24 @@ main() async { if (Platform.isMacOS && clangBuildToolsDir == null) { printSkip('ELF external stripping test'); } else { - // Test unstripped ELF generation that is then stripped externally. - await test(dillPath, stripUtil: true); - await test(dillPath, stripUtil: true, useBare: false); + // Test unstripped ELF generation that is then externally stripped. + await testAOT(aotDillPath, stripUtil: true); + await testAOT(aotDillPath, stripUtil: true, useBare: false); } // TODO(sstrickl): Currently we can't assemble for SIMARM64 on MacOSX. - // For example, the test runner still uses blobs for dartkp-mac-*-simarm64. - // Change assembleSnapshot and remove this check when we can. + // For example, the test runner still uses blobs for + // dartkp-mac-*-simarm64. Change assembleSnapshot and remove this check + // when we can. if (Platform.isMacOS && buildDir.endsWith('SIMARM64')) { printSkip('assembly tests'); return; } - - // Test unstripped assembly generation that is then compiled and stripped. - await test(dillPath, useAsm: true); - await test(dillPath, useAsm: true, useBare: false); - // Test stripped assembly generation that is then compiled and stripped. - await test(dillPath, useAsm: true, stripFlag: true); - await test(dillPath, useAsm: true, stripFlag: true, useBare: false); + // Test unstripped assembly generation that is then externally stripped. + await testAOT(aotDillPath, useAsm: true); + await testAOT(aotDillPath, useAsm: true, useBare: false); + // Test stripped assembly generation that is then externally stripped. + await testAOT(aotDillPath, useAsm: true, stripFlag: true); + await testAOT(aotDillPath, useAsm: true, stripFlag: true, useBare: false); }); } diff --git a/runtime/vm/image_snapshot.cc b/runtime/vm/image_snapshot.cc index 344ae2550c4..eaffe443d25 100644 --- a/runtime/vm/image_snapshot.cc +++ b/runtime/vm/image_snapshot.cc @@ -651,14 +651,14 @@ void ImageWriter::WriteText(bool vm) { const char* bss_symbol = SectionSymbol(ProgramSection::Bss, vm); ASSERT(bss_symbol != nullptr); - if (FLAG_precompiled_mode) { - if (profile_writer_ != nullptr) { - profile_writer_->SetObjectTypeAndName(parent_id, image_type_, - instructions_symbol); - profile_writer_->AttributeBytesTo(parent_id, Image::kHeaderSize); - profile_writer_->AddRoot(parent_id); - } + if (profile_writer_ != nullptr) { + profile_writer_->SetObjectTypeAndName(parent_id, image_type_, + instructions_symbol); + profile_writer_->AttributeBytesTo(parent_id, Image::kHeaderSize); + profile_writer_->AddRoot(parent_id); + } + if (FLAG_precompiled_mode) { const intptr_t section_header_length = compiler::target::InstructionsSection::HeaderSize(); // Calculated using next_text_offset_, which doesn't include post-payload diff --git a/runtime/vm/v8_snapshot_writer.cc b/runtime/vm/v8_snapshot_writer.cc index 6fb58987421..351c8319313 100644 --- a/runtime/vm/v8_snapshot_writer.cc +++ b/runtime/vm/v8_snapshot_writer.cc @@ -29,8 +29,6 @@ V8SnapshotProfileWriter::V8SnapshotProfileWriter(Zone* zone) idx = edge_types_.Add("internal"); ASSERT_EQUAL(idx, static_cast(Edge::Type::kInternal)); - unknown_type_string_index_ = node_types_.Add("Unknown"); - SetObjectTypeAndName(kArtificialRootId, "ArtificialRoot", ""); } @@ -113,14 +111,10 @@ const char* V8SnapshotProfileWriter::NodeInfo::ToCString(Zone* zone) const { void V8SnapshotProfileWriter::NodeInfo::Write(JSONWriter* writer) const { ASSERT(id.space() != IdSpace::kInvalid); - ASSERT(type != kInvalidString); if (type == kInvalidString) { - // Fall back on this string in non-DEBUG modes. See - // https://github.com/dart-lang/sdk/issues/45787 for context. - writer->PrintValue(profile_writer_->unknown_type_string_index_); - } else { - writer->PrintValue(type); + FATAL("No type given for node %s", id.ToCString(profile_writer_->zone_)); } + writer->PrintValue(type); if (name != kInvalidString) { writer->PrintValue(name); } else { diff --git a/runtime/vm/v8_snapshot_writer.h b/runtime/vm/v8_snapshot_writer.h index 9aededced1a..ea0a0e74b0e 100644 --- a/runtime/vm/v8_snapshot_writer.h +++ b/runtime/vm/v8_snapshot_writer.h @@ -331,7 +331,6 @@ class V8SnapshotProfileWriter : public ZoneAllocated { StringsTable edge_types_; StringsTable strings_; DirectChainedHashMap roots_; - intptr_t unknown_type_string_index_; #endif };