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

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

[phyBrackets]

Hi @jmorse , Do you have any opinions on this?

[phyBrackets]

ping

[jmorse]

Sorry for the very long delay, this sank a long way off my radar. Also sorry for the "I'm not sure" answer below,

The test originates from f60e0a1, where "empty" source-location allocas were being assigned an inlining-call-site source location so that they'd have something useful, and then this was leaking into stack smashing protection. This patch goes further by dropping the source locations from any inlined allocas at all.

If my thinking's right, we've got two competing interests of:

• The source location at any Instruction should be Good Enough (TM) for newly inserted instructions to inherit,
• The source location of these allocas should be True (which is a long lexical distance from the start of the function).

Where the stack-protection code suffers from relying on the former but suffering the latter, plus it could technically be implemented differently.

IMO: we could chose either path here, either dropping the source locations on allocas or implementing the stack-protection code differently. Dropping alloca locations is more liberal and is more likely to lead to unexpected outcomes (but also might fix more behaviours than just this), while changing stack-protection is conservative. In fact, if we were going for technical rigour, we should do both. I suppose the best question is: do you @phyBrackets feel up to fixing some of the unexpected outcomes that might result from landing this patch?

[phyBrackets]

Hi @jmorse Thanks for getting back to this, no worries on the delay!

you're right that dropping alloca locations is the more aggressive fix and could surface unexpected behaviors, but I'm up for dealing with those if they come up.

I think starting with this approach makes sense since it's more likely to catch other similar issues beyond just stack-protection. If we hit any weird edge cases or regressions, I'm happy to either fix them or pivot to the more conservative stack-protection specific fix if needed.

[phyBrackets]

Hi @jmorse , gentle ping on this.