IRBuilder: avoid crash when seeking to start of a BasicBlock with only DebugInfo

[durin42]

This fixes a crash in rustc that was triggered by
https://reviews.llvm.org/D159485 (aka
1ce1732).

This was more or less pair-programmed with @krasimirgg - I can't claim
full credit.

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-ir

Changes

This fixes a crash in `rustc` that was triggered by https://reviews.llvm.org/D159485 (aka 1ce1732). I can't claim to fully understand the fix - my efforts at debugging were stymied by the crashing lines getting optimzed too well, and when I disabled optimizations the crash managed to go away and be replaced by something more confusing. @krasimirgg wrote this and I tested it, and he asked me to submit it for review. -- Full diff: https://github.com//pull/66266.diff

1 Files Affected:

• (modified) llvm/lib/IR/Instruction.cpp (+5-2)

diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 6b0348f8f691fd4..37cce7729741745 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]

The windows ninja failure is at a high enough level I suspect it's misconfiguration and is wholly unrelated to this change.

[MaskRay]

LLVM coding standard prefers Instruction *next. With C++17, prefer:

if (const Instruction *Next = getNextNonDebugInstruction())
  return Next->getDebugLoc();

[MaskRay]

The new code seems to preserve the behavior prior to https://reviews.llvm.org/D159485 so seems safe. We should need proper tests for this case, perhaps @jmorse (https://reviews.llvm.org/D159485) can help on it. But I can give mine if Rust urgently needs this.

[pogo59]

In https://reviews.llvm.org/D159485 @jmorse asked for an example basic block that would cause this crash. If you provided that, it would probably form the core of a proper test for this patch.
It does seem odd to have a sequence of debug instructions with no terminator. It would be interesting to understand how that happened.

[jmorse]

Thanks for getting a patch together -- IMO it there's no next instruction, we should return an empty source location, i.e. return DebugLoc();. Presumably in a scenario where there are no debug-info intrinsics in the input, IRBuilder is inserting at end(), and thus doesn't have a source location to set on the inserted instructions? We should try fairly hard to ensure that we get the same source locations as when there are no debug intrinsics in the input.

I'm tempted to say that this isn't a legal use of IRBuilder because presumably the crashing input has no terminator instruction, however the whole point of IRBuilder is that it's dealing with incomplete/illegal basic blocks, and it'd be awkward to start making rules about different degrees of illegal blocks.

In terms of testing, would you be able to add another case to the "DebugLoc" test in llvm/unittests/IR/IRBuilderTest.cpp, I reckon inserting a dbg.label, erasing the terminator then creating another instruction at the dbg.label should cover this nicely.

[durin42]

Thanks for getting a patch together -- IMO it there's no next instruction, we should return an empty source location, i.e. return DebugLoc();.

I tried this, and that preserves the segfault.

We're working on extracting some sort of reduced input here, but if I disable optimizations to try and dump the information you requested I appear to invoke some sort of UB elder god and the test mysteriously passes.

[durin42]

@jmorse could you elaborate on

In terms of testing, would you be able to add another case to the "DebugLoc" test in llvm/unittests/IR/IRBuilderTest.cpp, I reckon inserting a dbg.label, erasing the terminator then creating another instruction at the dbg.label should cover this nicely.

please? I'm not at all conversant in this part of LLVM.

[durin42]

Also, if it helps: I did manage to get some dump() equivalent, but it means nothing to me: https://reviews.llvm.org/D159485#4646085

[jmorse]

Thanks for the IR sample on D159485 -- I think this is certainly a legitimate use of IRBuilder even if we're trying to diminish and eliminate debug intrinsics [0]. The unit test in #66491 looks good too -- combine that with the style fix and we're good to go!

[0] Coming soon to a compiler near you soon!

[durin42]

@jmorse rebased and rewritten - but I still can't return DebugInfo() in the case when getNextNonDebugInstruction() returns null. I've folded in the unit test, and I think cleaned up the style. Please take a look and let me know what you think?

Thanks!

[jmorse]

LGTM; on closer examination it's impossible to return an empty debugloc here because it's returning an lvalue reference, and we would be returning a temporary. It's not too important to have this behaviour as it only becomes important when comparing normal debug-info with non-intrinsic-based debug-info. That's something we can push back for now.

Thanks for putting the patch together!