[IRBuilder][DebugInfo] Don't pick DebugLocs for new instructions from debug intrinsics

[jmorse]

Summary

As discussed in D59272, LLVM sometimes uses the DebugLoc of a debug intrinsic as the DebugLoc for newly created instructions. This is undesirable, as the line number information for debug intrinsics is at best meaningless and worst misleading -- D59272 has a couple of examples of variable declaration line numbers gratuitously appearing in unrelated code.

This patch has the IRBuilder insertion-point-setting methods skip over debug intrinsics, and find a "Real" instruction to set the DebugLoc from. As this isn't related to a particular pass or transformation, I've added a unit test to check that this behaviour is preserved.

Advice on additional reviewers would be appreciated, IRBuilder seems pretty central, it's not clear who else should review, aside from "Everyone".

[jmorse]

Add the test change I forgot to stage (whoops) -- the AMDGPU test here doesn't have any non-meta instructions with line numbers. Prior to this patch the debug intrinsic line numbers leak in, which the test depends on. Fix this by giving the real instructions line numbers. (The test is for placement of dbg.values rather than line numbers anyway).

[bjope]

include/llvm/IR/IRBuilder.h

139

This could end up returning the end() iterator (so the assert above is now out-of-play).

150

You probably want to do this before checking for end() (to avoid reading debug loc from end()).

Another interesting thing here is that when setting the insertion point to the end of the BB (or rather after the last non-debug-intrinsic in the BB) the current debug location won't be updated. Is some user of this method relying on that. Should perhaps the current debug location be invalidated instead?

My thoughts are:

• Should we simply skip setting debug location when insertion point is a debug intrinsic?
• Or should we invalidate the debug location when insertion point is a debug intrinsic?
• Should perhaps these SetInsertPoint methods take a second argument, providing the debug location to set (only ~800 call sites to update...)?
• Or should we never do the side effect of updating debug location (just as many call sites to potentially update with explicit settings)? Notice that the SetInsertPoint(BasicBlock *TheBB) version does not set the current debug location. So at the moment it isn't obvious that SetInsertPoint always updates the current debug location.

Haven't really thought about what I'd prefer myself.

[vsk]

There's a tension here between finding a narrow solution to the invalid-location bug and having SetInsertPoint() do something principled with debug locations. See D39982 for a more detailed explanation of this. That previous attempt to change the IRBuilder API stalled because we couldn't find an automated way to fix up in-tree/out-of-tree uses (imho this is a hard problem).

I think it'd make sense to opt for a narrow solution here, but am not sure building on top of SetInsertPoint() is the way to go. What do folks think about this (possibly naive) alternative: if locations on debug intrinsics aren't meaningful, can we get rid of them (or use line 0 locations which just preserve scope info)? This would prevent SetInsertPoint from propagating misleading debug locations.

Edit: I think this alternative amounts to a version of D59272 that doesn't special-case stores.

[jmorse]

For some reason I hadn't clocked that the IRBuilder deals with blocks in a state of partial construction/correctness (duh), and had assumed there would always be a terminator in each block, whoops.

In D61198#1480767, @vsk wrote:

There's a tension here between finding a narrow solution to the invalid-location bug and having SetInsertPoint() do something principled with debug locations. See D39982 for a more detailed explanation of this.

Ouch, I have now been enlightened,

I think it'd make sense to opt for a narrow solution here, but am not sure building on top of SetInsertPoint() is the way to go. What do folks think about this (possibly naive) alternative: if locations on debug intrinsics aren't meaningful, can we get rid of them (or use line 0 locations which just preserve scope info)? This would prevent SetInsertPoint from propagating misleading debug locations.

That sounds like a plan -- this patch was supposed to just reduce the circumstances where D59272 was necessary, but as it seems to be a harder problem to solve than expected, we'll just have to rely on zero-line-nos-for-debug-intrinsics more. I've updated D59272 accordingly.

include/llvm/IR/IRBuilder.h

 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Intrinsics.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/AtomicOrdering.h"
 #include "llvm/Support/CBindingWrapping.h"
void SetInsertPoint(BasicBlock *TheBB) {
     InsertPt = BB->end();
   }
 
   /// This specifies that created instructions should be inserted before
   /// the specified instruction.
   void SetInsertPoint(Instruction *I) {
     BB = I->getParent();
     InsertPt = I->getIterator();
-    assert(InsertPt != BB->end() && "Can't read debug loc from end()");
-    SetCurrentDebugLocation(I->getDebugLoc());
+    assert(I->getIterator() != BB->end() && "Can't read debug loc from end()");
+    // Avoid taking DebugLocs from debug intrinsics
+    auto DbgSrcInst = skipDebugIntrinsics(InsertPt);
+    SetCurrentDebugLocation(DbgSrcInst->getDebugLoc());
   }
 
   /// This specifies that created instructions should be inserted at the
   /// specified point.
   void SetInsertPoint(BasicBlock *TheBB, BasicBlock::iterator IP) {
     BB = TheBB;
     InsertPt = IP;
-    if (IP != TheBB->end())
-      SetCurrentDebugLocation(IP->getDebugLoc());
+    if (IP != TheBB->end()) {
+      // Avoid taking DebugLocs from debug intrinsics
+      auto DbgSrcInst = skipDebugIntrinsics(InsertPt);
+      SetCurrentDebugLocation(DbgSrcInst->getDebugLoc());
+    }
   }
 
   /// Set location information used by debugging information.
   void SetCurrentDebugLocation(DebugLoc L) { CurDbgLocation = std::move(L); }
 
   /// Get location information used by debugging information.
   const DebugLoc &getCurrentDebugLocation() const { return CurDbgLocation; }
 

unittests/IR/IRBuilderTest.cpp

TEST_F(IRBuilderTest, DIBuilderMacro) {
   EXPECT_EQ(MN1, MF1->getRawElements());
 
   Elements.clear();
   Elements.push_back(MDef2);
   auto MN2 = MDTuple::get(Ctx, Elements);
   EXPECT_EQ(MN2, MF2->getRawElements());
   EXPECT_TRUE(verifyModule(*M));
 }
+
+// Test that IRBuilder won't read DebugLocs from debug intrinsics at the
+// insertion location.
+TEST_F(IRBuilderTest, InsertionDebugLoc) {
+  IRBuilder<> Builder(BB);
+  Value *V;
+  Instruction *I;
+
+  // Create debug environment for test.
+  DIBuilder DIB(*M);
+  auto File = DIB.createFile("bees", "/");
+  auto CU = DIB.createCompileUnit(dwarf::DW_LANG_C, File, "hands", true, "", 0);
+  auto Type = DIB.createSubroutineType(DIB.getOrCreateTypeArray(None));
+  auto Func = DIB.createFunction(
+      CU, "shoe", "", File, 1, Type, 1, DINode::FlagZero,
+      DISubprogram::SPFlagDefinition);
+  auto Lex = DIB.createLexicalBlock(Func, File, 1, 1);
+  auto DIFloat = DIB.createBasicType("float", 32, dwarf::DW_ATE_float);
+  auto DIExpr = DIB.createExpression();
+  auto LocalVar = DIB.createAutoVariable(Lex, "sandal", File, 2, DIFloat);
+
+  // Create our "Real code" (TM) debug location, on line 10.
+  DebugLoc NormalDebugLoc = DebugLoc::get(10, 1, Lex);
+  // Create a debug location specially for a dbg.value we'l create, on line 2.
+  auto DbgValLoc = DILocation::get(Ctx, 2, 3, Lex);
+
+  // Create some floating point operations with a dbg.value in the middle.
+  Builder.SetCurrentDebugLocation(NormalDebugLoc);
+  V = Builder.CreateLoad(GV->getValueType(), GV);
+  I = cast<Instruction>(Builder.CreateFAdd(V, V));
+  auto DbgVal = DIB.insertDbgValueIntrinsic(I, LocalVar, DIExpr, DbgValLoc, BB);
+  Builder.CreateStore(I, GV);
+  Builder.CreateRetVoid();
+
+  // Now insert an extra instruction at the location of the dbg.value.
+  // IRBuilder should seek and find the "Real" debug location, and ignore the
+  // location of the intervening dbg.value.
+  Builder.SetInsertPoint(DbgVal);
+  Instruction *I2 = cast<Instruction>(Builder.CreateFAdd(I, V));
+
+  // Additionally created FAdd should have the normal debug location.
+  EXPECT_FALSE(I2->getDebugLoc().get() == DbgValLoc);
+  EXPECT_TRUE(I2->getDebugLoc() == NormalDebugLoc);
+
+  DIB.finalize();
+}
 }