[DebugInfo][RemoveDIs] Avoid leaking trailing DPMarkers

[jmorse]

In the debug-info-splice implementation, we need to be careful to delete trailing DPMarkers from blocks when we splice their contents out. This is equivalent to removing the terminator from a block, then splicing the rest of it's contents to another block: any DPValues trailing at the end of the block get moved and we need to clean up afterwards.

Digging into this was particularly nasty because... if we don't delete the trailing DPValues, then blocks can be deallocated then reallocated, with the trailing DPMarker appearing to jump from one to the other. That wouldn't happen with a value handle, but then we wouldn't be able to put it in a map.

I think this motivates us needing a less raw-pointer based solution in the future.

This is sort of NFC because it just squelches asan leaking errors, it's really hard to write a test for though, hence there aren't any.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

In the debug-info-splice implementation, we need to be careful to delete trailing DPMarkers from blocks when we splice their contents out. This is equivalent to removing the terminator from a block, then splicing the rest of it's contents to another block: any DPValues trailing at the end of the block get moved and we need to clean up afterwards.

Digging into this was particularly nasty because... if we don't delete the trailing DPValues, then blocks can be deallocated then reallocated, with the trailing DPMarker appearing to jump from one to the other. That wouldn't happen with a value handle, but then we wouldn't be able to put it in a map.

I think this motivates us needing a less raw-pointer based solution in the future.

This is sort of NFC because it just squelches asan leaking errors, it's really hard to write a test for though, hence there aren't any.

---

Full diff: https://github.com/llvm/llvm-project/pull/74458.diff

1 Files Affected:

• (modified) llvm/lib/IR/BasicBlock.cpp (+9-1)

diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 3ac5fafd887df..872386ddb54b2 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -770,6 +770,7 @@ void BasicBlock::flushTerminatorDbgValues() {
 
   // Transfer DPValues from the trailing position onto the terminator.
   Term->DbgMarker->absorbDebugValues(*TrailingDPValues, false);
+  TrailingDPValues->eraseFromParent();
   deleteTrailingDPValues();
 }
 
@@ -813,6 +814,7 @@ void BasicBlock::spliceDebugInfoEmptyBlock(BasicBlock::iterator Dest,
 
     DPMarker *M = Dest->DbgMarker;
     M->absorbDebugValues(*SrcTrailingDPValues, InsertAtHead);
+    SrcTrailingDPValues->eraseFromParent();
     Src->deleteTrailingDPValues();
     return;
   }
@@ -920,6 +922,7 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src,
   // Use this flag to signal the abnormal case, where we don't want to copy the
   // DPValues ahead of the "Last" position.
   bool ReadFromTail = !Last.getTailBit();
+  bool LastIsEnd = (Last == Src->end());
 
   /*
     Here's an illustration of what we're about to do. We have two blocks, this
@@ -995,12 +998,16 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src,
     DPMarker *OntoDest = getMarker(Dest);
     DPMarker *FromLast = Src->getMarker(Last);
     OntoDest->absorbDebugValues(*FromLast, true);
+    if (LastIsEnd) {
+      FromLast->eraseFromParent();
+      deleteTrailingDPValues();
+    }
   }
 
   // If we're _not_ reading from the head of First, i.e. the "++++" DPValues,
   // move their markers onto Last. They remain in the Src block. No action
   // needed.
-  if (!ReadFromHead) {
+  if (!ReadFromHead && First->hasDbgValues()) {
     DPMarker *OntoLast = Src->createMarker(Last);
     DPMarker *FromFirst = Src->createMarker(First);
     OntoLast->absorbDebugValues(*FromFirst,
@@ -1030,6 +1037,7 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src,
     DPMarker *TrailingDPValues = getTrailingDPValues();
     if (TrailingDPValues) {
       FirstMarker->absorbDebugValues(*TrailingDPValues, true);
+      TrailingDPValues->eraseFromParent();
       deleteTrailingDPValues();
     }
   }

[OCHyams]

Should absorbDebugValues call eraseFromParent? Rather than calling it all the absorbDebugValues call sites. AFAICT absorb always absorbs all the DPValues, so it would probably be a rare case that we want to keep the absorbed-from marker around?

[jmorse]

Eventually yes -- however the RemoveDIs/DDD model in-tree right now has one DPMarker allocated per instruction, and we haven't landed the plumbing required to support that (we'll get null pointer exceptions everywhere otherwise).

Over in 2f93a72, which is a refactoring of the internal patch we've got, there's an "adopt" call that works very similarly to what you describe here. It might be able to automate erasing (erasal?) but still requires juggling with the trailingDPValues :(.

[OCHyams]

LGTM

think this motivates us needing a less raw-pointer based solution in the future.

SGTM