[DebugInfo] Fix incorrect debug attribution for inlined allocas

[phyBrackets]

Fixing #142662

Fix incorrect debug attribution for inlined allocas, Clear debug locations from all inlined alloca instructions to prevent stack protection and other generated code from inheriting incorrect source line attributions from the callee function.

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Shivam Kunwar (phyBrackets)

Changes

Fixing #142662

Fix incorrect debug attribution for inlined allocas, Clear debug locations from all inlined alloca instructions to prevent stack protection and other generated code from inheriting incorrect source line attributions from the callee function.

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Utils/InlineFunction.cpp (+10)

diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index 21467a909af10..312b4888ee6e7 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -2703,6 +2703,16 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI,
     // Remember the first block that is newly cloned over.
     FirstNewBlock = LastBlock; ++FirstNewBlock;
 
+    // Clear debug locations for all inlined allocas to prevent stack protection
+    // code from inheriting incorrect source attribution
+    for (Function::iterator BB = FirstNewBlock, E = Caller->end(); BB != E; ++BB) {
+     for (BasicBlock::iterator I = BB->begin(), IE = BB->end(); I != IE; ++I) {
+        if (auto *AI = dyn_cast<AllocaInst>(I)) {
+          AI->setDebugLoc(DebugLoc());
+        }
+      }
+    }
+
     // Insert retainRV/clainRV runtime calls.
     objcarc::ARCInstKind RVCallKind = objcarc::getAttachedARCFunctionKind(&CB);
     if (RVCallKind != objcarc::ARCInstKind::None)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[dwblaikie]

Please include a test case

[dwblaikie]

Mostly happy to defer this to @jmorse and @SLTozer - but I am a bit confused.

On the one hand, I'm not sure any allocas should have source locations? They all effectively get lumped into the start of the function and merged anyway, and it's in the prelude, and it's not interesting (unlikely to fail - ultimately just adding some offset to the stack/frame pointer)?

On the other hand - it is accurate to attribute the alloca to the inlined function - what's going wrong if we do that? Should that thing be going wrong? Should the stack protection code not have a location rather than inheriting one from an arbitrary alloca?

[SLTozer]

I have some small code comments, but mostly interested in the big picture stuff following what @dwblaikie has said:

On the one hand, I'm not sure any allocas should have source locations?

Most allocas don't have source locations, some allocas do - one example seems to be instructions generated for __builtin_alloca and similar intrinsics. I think I agree that in principle, we do not want to arbitrarily/unconditionally erase these source locations.

On the other hand, we may have a valid case for dropping them. If there are any conditional branches between the entry block and the inlined call, then we should drop the debug locations. It may be that as a matter of practicality, it's easier to simply unconditionally drop locations rather than traversing the CFG to determine this fact.

Finally, it seems like a separate issue altogether that the location of allocas in the entry block leaks onto stack protection instructions - dropping the locations on inlined allocas may fix that, but it may also be worth addressing directly. I suspect that directly applying a function-entry location to such instructions may be correct, but I'm not sure yet - would have to look into it a bit more.

[SLTozer]

Nit, full stop.

Suggested change

	// Clear debug locations for all inlined allocas to prevent stack protection
	// code from inheriting incorrect source attribution
	// Clear debug locations for all inlined allocas to prevent stack protection
	// code from inheriting incorrect source attribution.

[SLTozer]

Suggested change

	AI->setDebugLoc(DebugLoc());
	AI->setDebugLoc(DebugLoc::getDropped());
	// or...
	AI->dropLocation();

Some recent changes have landed to better track missing debug locations - see the docs and this RFC for more details, but the tl;dr is that instead of using DebugLoc() for empty locations we'd prefer an annotative location that categorizes the nature of the empty location.

[phyBrackets]

Thanks @SLTozer @dwblaikie for the inputs and review.

I agree that unconditionally clearing all alloca debug locations is too broad, I am not sure if we should handle directly it into stack protection pass by not inheriting debug locations from allocas , at this moment either the conditional approach sounds good, Clear debug locations only when there are conditional branches between the entry block and the call site, as you suggested. This preserves legitimate source locations (like __builtin_alloca) while fixing the attribution issue, but again traversing CFG comes with a cost, so definitely not an easy option, Or we should move with the current approach?

I will take a look at it, until any other opinions and suggestions are welcome.