[vm] Fix handling of WeakSerializationReferences in v8 profile.
Instead of trying to lazily create artificial nodes when needed for
WeakSerializationReference (WSR) targets in all cases, create them
eagerly for targets in reachable WSRs, since those are guaranteed
to be needed.
Those for unreachable WSRs are still lazily created as needed, since the
WSR won't even be accessed by the clustered snapshot writer unless the
unreachable WSR is part of another unreachable object that has an
artificial node created.
This rework avoids some issues seen on upcoming CLs where the artificial
nodes for WSR targets weren't getting correctly generated.
-----
Also extend the v8 snapshot profile writer tests to check the sizes of
the text and data sections in ELF snapshots. That means the v8 snapshot
profile writer tests check up to three different measures, from most
precise to least precise, depending on the output mode:
* If writing an ELF snapshot directly: the sum of the sizes attributed
to the text and data section symbols are checked to be exactly the
same as the sum of the sizes of the objects in the profile.
* If writing an ELF snapshot either directly or via assembly: the sum
of the sizes of the text and data sections are checked to be the same
as the sum of the sizes of the objects in the profile. If using an
assembler that merges text and data sections, then account for the
padding between the sections using an approximate check.
* For all: Check that the size of the snapshot as a whole is
approximately equal to the sum of the sizes of the objects in the
profile, accounting both for possible padding and for the global
header information and non-data/non-text sections like the dynamic
symbol section.
TEST=vm/data{,_2}/v8_snapshot_profile_writer_test
Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64c-try,vm-kernel-precomp-nnbd-linux-debug-x64-try,vm-kernel-precomp-mac-release-simarm64-try
Change-Id: I66e0e7fdb5bb98045621bf516f150a4723e08147
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198942
Commit-Queue: Tess Strickland <sstrickl@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
Reviewed-by: Daco Harkes <dacoharkes@google.com>
diff --git a/pkg/native_stack_traces/CHANGELOG.md b/pkg/native_stack_traces/CHANGELOG.md
index ae7d875..7c60f28 100644
--- a/pkg/native_stack_traces/CHANGELOG.md
+++ b/pkg/native_stack_traces/CHANGELOG.md
@@ -1,5 +1,9 @@
# Changelog
+## 0.4.3
+
+- Exported some more of the ELF utilities for use in Dart tests.
+
## 0.4.2
- When decoding a stack trace, frames corresponding to the functions
diff --git a/pkg/native_stack_traces/lib/elf.dart b/pkg/native_stack_traces/lib/elf.dart
index de32cd3..3b1624b 100644
--- a/pkg/native_stack_traces/lib/elf.dart
+++ b/pkg/native_stack_traces/lib/elf.dart
@@ -2,7 +2,7 @@
// 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/elf.dart' show Elf, Section, Symbol;
export 'src/constants.dart'
show
isolateDataSymbolName,
diff --git a/pkg/native_stack_traces/pubspec.yaml b/pkg/native_stack_traces/pubspec.yaml
index d398708..bdf1f94 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.2
+version: 0.4.3
homepage: https://github.com/dart-lang/sdk/tree/master/pkg/native_stack_traces
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 8719b76..212571a 100644
--- a/runtime/tests/vm/dart/v8_snapshot_profile_writer_test.dart
+++ b/runtime/tests/vm/dart/v8_snapshot_profile_writer_test.dart
@@ -189,12 +189,17 @@
final expectedSize =
profile.nodes.fold<int>(0, (size, n) => size + n.selfSize);
+ // May not be ELF, but another format.
+ final elf = Elf.fromFile(snapshotPath);
+
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);
+ // Verify that the total size of the snapshot text and data section
+ // symbols 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.
+ //
+ // We only check this when generating ELF directly because that's when
+ // we're guaranteed the symbols will have non-zero size.
Expect.isNotNull(elf);
elf!; // To refine type to non-nullable version.
@@ -215,10 +220,54 @@
Expect.equals(
expectedSize, actualSize, "failed on $description snapshot");
+ Expect.equals(expectedSize, actualSize,
+ "symbol size check failed on $description snapshot");
+ checkedSize = true;
+ }
+
+ // See Elf::kPages in runtime/vm/elf.h, which is also used for assembly
+ // padding.
+ final segmentAlignment = 16 * 1024;
+
+ if (elf != null) {
+ // Verify that the total size of the snapshot text and data sections is
+ // approximately the sum of the shallow sizes of all objects in the
+ // profile. As sections might be merged by the assembler when useAsm is
+ // true, we need to account for possible padding.
+ final textSections = elf.namedSections(".text");
+ Expect.isNotEmpty(textSections);
+ Expect.isTrue(
+ textSections.length <= 2, "More text sections than expected");
+ final dataSections = elf.namedSections(".rodata");
+ Expect.isNotEmpty(dataSections);
+ Expect.isTrue(
+ dataSections.length <= 2, "More data sections than expected");
+
+ var actualSize = 0;
+ for (final section in textSections) {
+ actualSize += section.length;
+ }
+ for (final section in dataSections) {
+ actualSize += section.length;
+ }
+
+ final mergedCount = (2 - textSections.length) + (2 - dataSections.length);
+ final possiblePadding = mergedCount * segmentAlignment;
+
+ Expect.approxEquals(
+ expectedSize,
+ actualSize,
+ possiblePadding,
+ "section size failed on $description snapshot" +
+ (!useAsm ? ", but symbol size test passed" : ""));
checkedSize = true;
}
if (stripUtil || stripFlag) {
+ // 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, padding, and non-text/data
+ // sections.
var strippedSnapshotPath = snapshotPath;
if (stripUtil) {
strippedSnapshotPath = snapshotPath + '.stripped';
@@ -227,20 +276,18 @@
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");
+ Expect.approxEquals(
+ expectedSize,
+ actualSize,
+ tolerance,
+ "total size check failed on $description snapshot" +
+ (elf != null ? ", but section size checks passed" : ""));
checkedSize = true;
}
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 0628aa0..9b7c95d 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
@@ -197,12 +197,17 @@
final expectedSize =
profile.nodes.fold<int>(0, (size, n) => size + n.selfSize);
+ // May not be ELF, but another format.
+ final elf = Elf.fromFile(snapshotPath);
+
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);
+ // Verify that the total size of the snapshot text and data section
+ // symbols 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.
+ //
+ // We only check this when generating ELF directly because that's when
+ // we're guaranteed the symbols will have non-zero size.
Expect.isNotNull(elf);
final vmTextSectionSymbol = elf.dynamicSymbolFor(vmSymbolName);
@@ -220,12 +225,54 @@
isolateTextSectionSymbol.size +
isolateDataSectionSymbol.size;
- Expect.equals(
- expectedSize, actualSize, "failed on $description snapshot");
+ Expect.equals(expectedSize, actualSize,
+ "symbol size check failed on $description snapshot");
+ checkedSize = true;
+ }
+
+ // See Elf::kPages in runtime/vm/elf.h, which is also used for assembly
+ // padding.
+ final segmentAlignment = 16 * 1024;
+
+ if (elf != null) {
+ // Verify that the total size of the snapshot text and data sections is
+ // approximately the sum of the shallow sizes of all objects in the
+ // profile. As sections might be merged by the assembler when useAsm is
+ // true, we need to account for possible padding.
+ final textSections = elf.namedSections(".text");
+ Expect.isNotEmpty(textSections);
+ Expect.isTrue(
+ textSections.length <= 2, "More text sections than expected");
+ final dataSections = elf.namedSections(".rodata");
+ Expect.isNotEmpty(dataSections);
+ Expect.isTrue(
+ dataSections.length <= 2, "More data sections than expected");
+
+ var actualSize = 0;
+ for (final section in textSections) {
+ actualSize += section.length;
+ }
+ for (final section in dataSections) {
+ actualSize += section.length;
+ }
+
+ final mergedCount = (2 - textSections.length) + (2 - dataSections.length);
+ final possiblePadding = mergedCount * segmentAlignment;
+
+ Expect.approxEquals(
+ expectedSize,
+ actualSize,
+ possiblePadding,
+ "section size failed on $description snapshot" +
+ (!useAsm ? ", but symbol size test passed" : ""));
checkedSize = true;
}
if (stripUtil || stripFlag) {
+ // 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, padding, and non-text/data
+ // sections.
var strippedSnapshotPath = snapshotPath;
if (stripUtil) {
strippedSnapshotPath = snapshotPath + '.stripped';
@@ -234,20 +281,18 @@
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");
+ Expect.approxEquals(
+ expectedSize,
+ actualSize,
+ tolerance,
+ "total size check failed on $description snapshot" +
+ (elf != null ? ", but section size checks passed" : ""));
checkedSize = true;
}
diff --git a/runtime/vm/clustered_snapshot.cc b/runtime/vm/clustered_snapshot.cc
index b99bc8a..6802b92 100644
--- a/runtime/vm/clustered_snapshot.cc
+++ b/runtime/vm/clustered_snapshot.cc
@@ -2481,6 +2481,13 @@
intptr_t Count(Serializer* s) { return objects_.length(); }
+ void CreateArtificialTargetNodesIfNeeded(Serializer* s) {
+ for (intptr_t i = 0; i < objects_.length(); i++) {
+ WeakSerializationReferencePtr weak = objects_[i];
+ s->CreateArtificialNodeIfNeeded(weak->untag()->target());
+ }
+ }
+
void WriteAlloc(Serializer* s) {
UNREACHABLE(); // No WSRs are serialized, and so this cluster is not added.
}
@@ -6268,34 +6275,29 @@
ObjectPtr object,
const V8SnapshotProfileWriter::Reference& reference) {
if (profile_writer_ == nullptr) return;
-
+ const auto& object_id = GetProfileId(object);
#if defined(DART_PRECOMPILER)
- // Make artificial nodes for dropped targets in WSRs.
if (object->IsHeapObject() && object->IsWeakSerializationReference()) {
- const auto& wsr = WeakSerializationReference::RawCast(object);
- const auto& target = wsr->untag()->target();
- const bool wsr_reachable = !CreateArtificialNodeIfNeeded(wsr);
- if (wsr_reachable && HasArtificialRef(target)) {
- // The target has artificial information used for snapshot analysis and
- // the replacement is part of the snapshot, so write information for both.
- const auto& replacement = wsr->untag()->replacement();
+ auto const wsr = WeakSerializationReference::RawCast(object);
+ auto const target = wsr->untag()->target();
+ const auto& target_id = GetProfileId(target);
+ if (object_id != target_id) {
+ const auto& replacement_id = GetProfileId(wsr->untag()->replacement());
+ ASSERT(object_id == replacement_id);
+ // The target of the WSR will be replaced in the snapshot, so write
+ // attributions for both the dropped target and for the replacement.
profile_writer_->AttributeDroppedReferenceTo(
- object_currently_writing_.id_, reference, GetProfileId(target),
- GetProfileId(replacement));
+ object_currently_writing_.id_, reference, target_id, replacement_id);
return;
}
- // The replacement isn't used, as either the target is strongly referenced
- // or the WSR itself is unreachable, so fall through to attributing a
- // reference to the WSR (which shares a profile ID with the target).
- ASSERT(GetProfileId(wsr) == GetProfileId(target));
- } else if (object_currently_writing_.id_.IsArtificial()) {
- // We may need to recur when writing members of artificial nodes in
- // CreateArtificialNodeIfNeeded.
- CreateArtificialNodeIfNeeded(object);
+ // The replacement isn't used for this WSR in the snapshot, as either the
+ // target is strongly referenced or the WSR itself is unreachable, so fall
+ // through to attributing a reference to the WSR (which shares the profile
+ // ID of the target).
}
#endif
profile_writer_->AttributeReferenceTo(object_currently_writing_.id_,
- reference, GetProfileId(object));
+ reference, object_id);
}
Serializer::WritingObjectScope::WritingObjectScope(
@@ -6364,33 +6366,19 @@
// UnsafeRefId will do lazy reference allocation for WSRs.
intptr_t id = UnsafeRefId(obj);
ASSERT(id != kUnallocatedReference);
- if (IsArtificialReference(id)) {
- return true;
+ if (id != kUnreachableReference) {
+ return IsArtificialReference(id);
}
if (obj->IsHeapObject() && obj->IsWeakSerializationReference()) {
- // The object ID for the WSR may need lazy resolution.
- if (id == kUnallocatedReference) {
- id = UnsafeRefId(obj);
- }
- ASSERT(id != kUnallocatedReference);
- // Create an artificial node for an unreachable target at this point,
- // whether or not the WSR itself is reachable.
- const auto& target =
+ auto const target =
WeakSerializationReference::RawCast(obj)->untag()->target();
CreateArtificialNodeIfNeeded(target);
- if (id == kUnreachableReference) {
- ASSERT(HasArtificialRef(target));
- // We can safely set the WSR's object ID to the target's artificial one,
- // as that won't make it look reachable.
- heap_->SetObjectId(obj, heap_->GetObjectId(target));
- return true;
- }
- // The WSR is reachable, so continue to the IsAllocatedReference behavior.
+ // Since the WSR is unreachable, we can replace its id with whatever the
+ // ID of the target is, whether real or artificial.
+ id = heap_->GetObjectId(target);
+ heap_->SetObjectId(obj, id);
+ return IsArtificialReference(id);
}
- if (IsAllocatedReference(id)) {
- return false;
- }
- ASSERT_EQUAL(id, kUnreachableReference);
const char* type = nullptr;
const char* name = nullptr;
@@ -6490,6 +6478,7 @@
id = AssignArtificialRef(obj);
Serializer::WritingObjectScope scope(this, type, obj, name);
for (const auto& link : links) {
+ CreateArtificialNodeIfNeeded(link.first);
AttributeReference(link.first, link.second);
}
return true;
@@ -7045,10 +7034,11 @@
}
#if defined(DART_PRECOMPILER)
- if (auto const cluster = CID_CLUSTER(WeakSerializationReference)) {
+ auto const wsr_cluster = CID_CLUSTER(WeakSerializationReference);
+ if (wsr_cluster != nullptr) {
// Now that we have computed the reachability fixpoint, we remove the
// count of now-reachable WSRs as they are not actually serialized.
- num_written_objects_ -= cluster->Count(this);
+ num_written_objects_ -= wsr_cluster->Count(this);
// We don't need to write this cluster, so remove it from consideration.
clusters_by_cid_[kWeakSerializationReferenceCid] = nullptr;
}
@@ -7137,6 +7127,19 @@
// And recorded them all in [objects_].
ASSERT(objects_->length() == num_objects);
+#if defined(DART_PRECOMPILER)
+ if (profile_writer_ != nullptr && wsr_cluster != nullptr) {
+ // Post-WriteAlloc, we eagerly create artificial nodes for any unreachable
+ // targets in reachable WSRs if writing a v8 snapshot profile, since they
+ // will be used in AttributeReference().
+ //
+ // Unreachable WSRs may also need artifical nodes, as they may be members
+ // of other unreachable objects that have artificial nodes in the profile,
+ // but they are instead lazily handled in CreateArtificialNodeIfNeeded().
+ wsr_cluster->CreateArtificialTargetNodesIfNeeded(this);
+ }
+#endif
+
for (SerializationCluster* cluster : clusters) {
cluster->WriteAndMeasureFill(this);
#if defined(DEBUG)
