[RemoveDIs][DebugInfo] Correctly visit DPValues in StackInfoBuilder::visit

[SLTozer]

In StackInfoBuilder::visit(Instruction &Inst), operations are performed on memory-related instructions, including debug intrinsics that refer to "interesting" allocas. There is a block that also visits DPValues attached to the instruction, but this block is near the end of the function; this has two problems:

1. The DPValues attached to an instruction precede that instruction, so they should always be processed before the instruction itself.
2. More importantly, some of the paths for visiting other instructions contain early returns, which will result in the DPValues not being visited at all.

This patch simply moves the DPValue-visiting block to the top of the function, which should resolve both of these problems.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Stephen Tozer (SLTozer)

Changes

In StackInfoBuilder::visit(Instruction &Inst), operations are performed on memory-related instructions, including debug intrinsics that refer to "interesting" allocas. There is a block that also visits DPValues attached to the instruction, but this block is near the end of the function; this has two problems:

1. The DPValues attached to an instruction precede that instruction, so they should always be processed before the instruction itself.
2. More importantly, some of the paths for visiting other instructions contain early returns, which will result in the DPValues not being visited at all.
   This patch simply moves the DPValue-visiting block to the top of the function, which should resolve both of these problems.

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp (+14-14)

diff --git a/llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp b/llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp
index 648a5276a3d60a..43366950d8df1e 100644
--- a/llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp
+++ b/llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp
@@ -110,6 +110,20 @@ Instruction *getUntagLocationIfFunctionExit(Instruction &Inst) {
 }
 
 void StackInfoBuilder::visit(Instruction &Inst) {
+  // Check for non-intrinsic debug-info records.
+  for (auto &DPV : Inst.getDbgValueRange()) {
+    for (Value *V : DPV.location_ops()) {
+      if (auto *AI = dyn_cast_or_null<AllocaInst>(V)) {
+        if (!isInterestingAlloca(*AI))
+          continue;
+        AllocaInfo &AInfo = Info.AllocasToInstrument[AI];
+        auto &DPVVec = AInfo.DbgVariableRecords;
+        if (DPVVec.empty() || DPVVec.back() != &DPV)
+          DPVVec.push_back(&DPV);
+      }
+    }
+  }
+
   if (CallInst *CI = dyn_cast<CallInst>(&Inst)) {
     if (CI->canReturnTwice()) {
       Info.CallsReturnTwice = true;
@@ -150,20 +164,6 @@ void StackInfoBuilder::visit(Instruction &Inst) {
     }
   }
 
-  // Check for non-intrinsic debug-info records.
-  for (auto &DPV : Inst.getDbgValueRange()) {
-    for (Value *V : DPV.location_ops()) {
-      if (auto *AI = dyn_cast_or_null<AllocaInst>(V)) {
-        if (!isInterestingAlloca(*AI))
-          continue;
-        AllocaInfo &AInfo = Info.AllocasToInstrument[AI];
-        auto &DPVVec = AInfo.DbgVariableRecords;
-        if (DPVVec.empty() || DPVVec.back() != &DPV)
-          DPVVec.push_back(&DPV);
-      }
-    }
-  }
-
   Instruction *ExitUntag = getUntagLocationIfFunctionExit(Inst);
   if (ExitUntag)
     Info.RetVec.push_back(ExitUntag);

[jmorse]

LGTM, but could we get a regression test too to prevent this from happening in the future? Should be achievable either by inserting a lifetime intrinsic into an existing test, or duplicating an existing function.

[fmayer]

LGTM, also would like a test.

More importantly, some of the paths for visiting other instructions contain early returns, which will result in the DPValues not being visited at all.

Okay for now, but we should restructure the function to not make this happen by design, not by ordering. I can do that when I get some time.

[SLTozer]

I've modified an existing test to trigger for this, with a comment to explain the purpose of the test - more tests could potentially be covered here, but none as conveniently as the test that I've added, and I would hazard that more testing is unnecessary for this error since there are also other tests (that I can't run locally right now) that will also fail if RemoveDIs is enabled by default.

[jmorse]

LGTM; this is probably the start of a theme of "now passes can more easily forget about debug-info", which is the natural result of making it less intrusive. Testing for all of these things is the only surefire solution.