Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[DebugInfo][RemoveDIs] Use getStableDebugLoc when picking IRBuilder source locations

Authored by jmorse on Sep 8 2023, 7:56 AM.




This is a patch broken out of D152468 and D151419 that deploys the "getStableDebugLoc" method -- this is a version of getDebugLoc that skips past any debug intrinsics to find a "real instruction" DebugLoc. Without this change, we can set real-instruction source locations to source locations that come from debug intrinsics, which aren't reliable and might vary with compiler flags. It turns out there are a few tests that are sensitive to this, the changes to which are elaborated below.

Firstly llvm/test/CodeGen/AMDGPU/llvm.dbg.value.ll (CC @arsenm and @scott.linder ) -- it appears that this is testing that the assembly syntax for a DWARF variable location is emitted correctly, but none of the "real" instructions have source locations. The test seems to implicitly rely on the source locations in the dbg.value leaking into the adjacent instructions, so that there's a lexical scope to cover. I've added source locations to the other IR instructions here to make the test pass again, would you agree this is the correct fix?

llvm/test/Transforms/SROA/vector-promotion.ll : The arithmetic instructions added were taking their source location from the dbg.value at the end of the function; they now take it from the "ret" instruction. This still makes sense, as the "ret" is the SSA Value Use that causes the instructions to be generated. The bitcast gets a different source location, corresponding to the "load" instruction in the source IR: I think this makes sense as that's what's converting the vector into an integer, through some stack store/loading.

LoopIdiom/X86/logical-right-shift-until-zero-debuginfo.ll and the simiarly named file: previously the generated instructions got the source location of the PHI in the loop because that's what was attached to the nearby dbg.value. Now the generated instructions get the source location of the first "real" instruction in the loop, which makes more sense to me.

hwaddresssanitizer/alloca.ll: All the hwasan generated instrs were getting the line number from the dbg.value, i.e. zero, which is liable to annoy developers. Now it comes from the call to use32, which I believe is what causes HWASAN to add instrumentation anyway. Added check-globals in the hope that in the future people will notice if those instructions become zero-line-numbered then someone will notice.

Diff Detail

Event Timeline

jmorse created this revision.Sep 8 2023, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2023, 7:56 AM
jmorse requested review of this revision.Sep 8 2023, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2023, 7:56 AM

LGTM, including agreeing with your assessment of and changes to llvm/test/CodeGen/AMDGPU/llvm.dbg.value.ll (comments from @arsenm or @scott.linder to confirm would still be welcome of course, if either of you have the bandwidth)


Worth duplicating the comment you added to the other similar hwasan test IMO.

Orlando accepted this revision.Sep 11 2023, 7:28 AM

(forgot to hit Accept)

This revision is now accepted and ready to land.Sep 11 2023, 7:28 AM

LGTM, including agreeing with your assessment of and changes to llvm/test/CodeGen/AMDGPU/llvm.dbg.value.ll (comments from @arsenm or @scott.linder to confirm would still be welcome of course, if either of you have the bandwidth)

Yes, that LGTM, thank you!

I'm working on more details, but I'm getting a SEGV in rustc when built against LLVM with this change. Bisect said this specific rev was the culprit, though that doesn't rule out it being a misuse of something in rustc.

This revision will cause more of the instruction chain to be examined, and I suppose that if you have a block containing nothing but debug intrinsics and no terminator, it might walk off the end iterator? (Or perhaps there's just a mistake I've made).

durin42 added inline comments.Sep 12 2023, 1:03 PM

The crash I'm seeing is on this line, I can give you the stack trace if you want to look in parallel with my investigation.

jmorse added inline comments.Sep 12 2023, 1:07 PM

Rather than the stack, the contents of the block would be super-useful -- if you can attach an interactive debugger before the crash, then when the crash occurs, running:

call IP->dump()
call IP->getParent()->dump()

Will print all the relevant information to the terminal / output

durin42 added inline comments.Sep 13 2023, 9:40 AM

How does this as a follow-up look to you?

diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 6b0348f8f691..37cce7729741 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -886,8 +886,11 @@ Instruction::getPrevNonDebugInstruction(bool SkipPseudoOp) const {

 const DebugLoc &Instruction::getStableDebugLoc() const {
-  if (isa<DbgInfoIntrinsic>(this))
-    return getNextNonDebugInstruction()->getDebugLoc();
+  if (isa<DbgInfoIntrinsic>(this)) {
+    const Instruction* next = getNextNonDebugInstruction();
+    if (next)
+      return next->getDebugLoc();
+  }
   return getDebugLoc();
durin42 added inline comments.Sep 14 2023, 10:07 AM

Finally got the dump (had to write it to a file in /tmp, sigh):

  call void @llvm.dbg.declare(metadata ptr %x, metadata !4697, metadata !DIExpression()), !dbg !4698

  call void @llvm.dbg.declare(metadata ptr %x, metadata !562, metadata !DIExpression()), !dbg !563

Also: I'm unable to get IR from rustc that can trigger this - the IR (even at -O0 equivalent) from a working rustc is unable to trigger the segfault.

aeubanks added inline comments.

the rustc backtrace was trying to create an alloca, is the issue that the dbg intrinsic is getting built before the alloca? so maybe an issue on the rustc side? it's weird that the entry block (I'm assuming based on start:) only has a dbg.declare without a corresponding alloca already there

Prepped an IR test case in based roughly on what rust was doing around the point of the segfault: create a basic block with just a debug call.

Not a rust compiler expert. From what I can gather looking at the wrapping code, rustc uses an utility LLVMRustPositionBuilderAtStart:

extern "C" void LLVMRustPositionBuilderAtStart(LLVMBuilderRef B,
                                               LLVMBasicBlockRef BB) {
  auto Point = unwrap(BB)->getFirstInsertionPt();
  unwrap(B)->SetInsertPoint(unwrap(BB), Point);

At the point of the crash, rust is operating on the start basic block of a function dump like this:

; Function Attrs: alwaysinline nonlazybind uwtable
define internal noundef nonnull align 8 ptr @"_ZN5alloc5boxed12Box$LT$T$GT$3new17h3257ceec0d2279b7E"(ptr noalias nocapture noundef align 8 dereferenceable(48) %x) unnamed_addr #4 personality ptr @rust_eh_personality !dbg !4808 {
  call void @llvm.dbg.declare(metadata ptr %x, metadata !4812, metadata !DIExpression()), !dbg !4813

bb2:                                              ; No predecessors!

cleanup:                                          ; No predecessors!
  %0 = landingpad { ptr, i32 }
  %1 = extractvalue { ptr, i32 } %0, 0
  %2 = extractvalue { ptr, i32 } %0, 1

I suppose rust has the logic to emit debug info and to put the alloca instruction separately, in a way that the above is later turned into a more "standard" basic block.