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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
jmorse added reviewers: aprantl, vsk, bjope, dblaikie.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

As discussed in D59272 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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".


Repository:
  rL LLVM

https://reviews.llvm.org/D61198

Files:
  include/llvm/IR/IRBuilder.h
  unittests/IR/IRBuilderTest.cpp


Index: unittests/IR/IRBuilderTest.cpp
===================================================================
--- unittests/IR/IRBuilderTest.cpp
+++ unittests/IR/IRBuilderTest.cpp
@@ -722,4 +722,50 @@
   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();
+}
 }
Index: include/llvm/IR/IRBuilder.h
===================================================================
--- include/llvm/IR/IRBuilder.h
+++ include/llvm/IR/IRBuilder.h
@@ -32,6 +32,7 @@
 #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"
@@ -133,8 +134,10 @@
   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
@@ -142,8 +145,11 @@
   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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61198.196867.patch
Type: text/x-patch
Size: 3729 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190426/705a67f3/attachment.bin>