[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>

Author: sstrickl

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

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,

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
 

runtime/tests/vm/dart/v8_snapshot_profile_writer_test.dart

@@ -189,12 +189,17 @@ Future<void> testAOT(String dillPath,
     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 @@ Future<void> testAOT(String dillPath,
 
       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 @@ Future<void> testAOT(String dillPath,
         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;
     }
 

runtime/tests/vm/dart_2/v8_snapshot_profile_writer_test.dart

@@ -197,12 +197,17 @@ Future<void> testAOT(String dillPath,
     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 @@ Future<void> testAOT(String dillPath,
           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 @@ Future<void> testAOT(String dillPath,
         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;
     }
 

runtime/vm/clustered_snapshot.cc

@@ -2481,6 +2481,13 @@ class WeakSerializationReferenceSerializationCluster
 
   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 @@ void Serializer::AttributeReference(
     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 @@ bool Serializer::CreateArtificialNodeIfNeeded(ObjectPtr obj) {
   // 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 @@ bool Serializer::CreateArtificialNodeIfNeeded(ObjectPtr obj) {
   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 @@ ZoneGrowableArray<Object*>* Serializer::Serialize(SerializationRoots* roots) {
   }
 
 #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 @@ ZoneGrowableArray<Object*>* Serializer::Serialize(SerializationRoots* roots) {
   // 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)