Ensure debug info for two calls to the same function from the same location are not merged

[dblaikie]

Summary

Starting in r176895 Clang began including column information on function calls in an attempt to fix inline debug info - if two calls from the same location (prior to r176895 this meant the same line, after r176895 it meant the same line and column) were both inlined, they'd get coalesced and treated as a single inline call ({call location} x {original location} was the uniqueness used to differentiate callers, if this was the same, they were the same call).

This solution (doubly so after it was narrowed in r177164) is incomplete (it wasn't applied to constructor calls (where it is still causing issues, such as the one worked around in r220923/PR21408) or member function calls) and insufficient: in the worst case, two calls from within a macro will be attributed to both the same line and column. It also produces possibly surprising and unintended side effects by adding column information to calls (increasing debug info size, surprising debuggers that aren't necessarily ready to cope with this, increasing the size of line tables, etc).

So, I present herein, a more robust solution to the problem. When inlining, uniquely identify the metadata associated with the inlined location ("InlinedAt") by adding an extra field to the end with a circular reference. In this way no two callers can be conflated together.

Once this is committed, I'll remove the hacks/workarounds from Clang. (& there's a test in Clang (test/CodeGenCXX/debug-info-same-line.cpp) that's currently testing this stuff in LLVM, so it'll need to be changed at the same time as this LLVM change (or removed/modified before it))

[dblaikie]

lib/Transforms/Utils/InlineFunction.cpp

829

Honestly I find this recursive loop function a bit hard to follow & wonder about refactoring it into a loop.

838

This code duplication I'm introducing here should be refactored away before the change is submitted, but I haven't decided how - open to ideas. (the obvious one is just to write a small utility function, but I'm wondering if it can be more refactored-by-design)

[echristo]

A few comments inline, but this type of problem is why we have discriminators as well - to be able to distinguish between two things that happen at the same location - this isn't the case for that, those are designed around things that happen at the same file/line/column but are in different basic blocks.

But essentially you're adding a unique discriminator to each inlined call which works for me to solve the problem.

lib/Transforms/Utils/InlineFunction.cpp

829

I think naming changes might make it easier, but I have no problem with you removing the recursion either.

866

Needs some comments here, it's a bit hard to follow the new logic. Maybe document the entire function algorithm now.

[dblaikie]

This ended up being fixed by df706288fba670943e164893e1fa0ccda3e24895

lib/Transforms/Utils/InlineFunction.cpp

for (User *U : AI->users()) {
     if (isUsedByLifetimeMarker(U))
       return true;
   }
   return false;
 }
 
 /// updateInlinedAtInfo - Helper function used by fixupLineNumbers to
 /// recursively update InlinedAtEntry of a DebugLoc.
-static DebugLoc updateInlinedAtInfo(const DebugLoc &DL, 
-                                    const DebugLoc &InlinedAtDL,
-                                    LLVMContext &Ctx) {
+static DebugLoc updateInlinedAtInfo(const DebugLoc &DL, MDNode *InlinedAtNode,
+                                    LLVMContext &Ctx,
+                                    DenseMap<MDNode *, MDNode *> &IANodes) {
   if (MDNode *IA = DL.getInlinedAt(Ctx)) {
-    DebugLoc NewInlinedAtDL 
-      = updateInlinedAtInfo(DebugLoc::getFromDILocation(IA), InlinedAtDL, Ctx);
+    auto *&IAUniqueNode = IANodes[IA];
+    if (!IAUniqueNode) {
+      DebugLoc NewInlinedAtDL = updateInlinedAtInfo(
+          DebugLoc::getFromDILocation(IA), InlinedAtNode, Ctx, IANodes);
+
+      InlinedAtNode = NewInlinedAtDL.getAsMDNode(Ctx);
+      MDNode *TempNode = MDNode::getTemporary(Ctx, None);
+      SmallVector<Value *, 5> Vals;
+      Vals.reserve(InlinedAtNode->getNumOperands() + 1);
+      for (unsigned i = 0; i != InlinedAtNode->getNumOperands(); ++i)
+        Vals.push_back(InlinedAtNode->getOperand(i));
+      Vals.push_back(TempNode);
+      IAUniqueNode = MDNode::get(Ctx, Vals);
+      IAUniqueNode->replaceOperandWith(Vals.size() - 1, IAUniqueNode);
+      MDNode::deleteTemporary(TempNode);
+    }
+
     return DebugLoc::get(DL.getLine(), DL.getCol(), DL.getScope(Ctx),
-                         NewInlinedAtDL.getAsMDNode(Ctx));
+                         IAUniqueNode);
   }
 
   return DebugLoc::get(DL.getLine(), DL.getCol(), DL.getScope(Ctx),
-                       InlinedAtDL.getAsMDNode(Ctx));
+                       InlinedAtNode);
 }
 
 /// fixupLineNumbers - Update inlined instructions' line numbers to 
 /// to encode location where these instructions are inlined.
 static void fixupLineNumbers(Function *Fn, Function::iterator FI,
                              Instruction *TheCall) {
   DebugLoc TheCallDL = TheCall->getDebugLoc();
   if (TheCallDL.isUnknown())
     return;
 
+  auto &Ctx = Fn->getContext();
+  auto *InlinedAtNode = TheCallDL.getAsMDNode(Ctx);
+  MDNode *TempNode = MDNode::getTemporary(Ctx, None);
+  SmallVector<Value *, 5> Vals;
+  Vals.reserve(InlinedAtNode->getNumOperands() + 1);
+  for (unsigned i = 0; i != InlinedAtNode->getNumOperands(); ++i)
+    Vals.push_back(InlinedAtNode->getOperand(i));
+  Vals.push_back(TempNode);
+  InlinedAtNode = MDNode::get(Ctx, Vals);
+  InlinedAtNode->replaceOperandWith(Vals.size() - 1, InlinedAtNode);
+  MDNode::deleteTemporary(TempNode);
+
+  DenseMap<MDNode *, MDNode *> IANodes;
+
   for (; FI != Fn->end(); ++FI) {
     for (BasicBlock::iterator BI = FI->begin(), BE = FI->end();
          BI != BE; ++BI) {
       DebugLoc DL = BI->getDebugLoc();
       if (DL.isUnknown()) {
         // If the inlined instruction has no line number, make it look as if it
         // originates from the call location. This is important for
         // ((__always_inline__, __nodebug__)) functions which must use caller
         // location for all instructions in their function body.
 
         // Don't update static allocas, as they may get moved later.
         if (auto *AI = dyn_cast<AllocaInst>(BI))
           if (isa<Constant>(AI->getArraySize()))
             continue;
 
         BI->setDebugLoc(TheCallDL);
       } else {
-        BI->setDebugLoc(updateInlinedAtInfo(DL, TheCallDL, BI->getContext()));
+        BI->setDebugLoc(updateInlinedAtInfo(DL, InlinedAtNode, Ctx, IANodes));
         if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(BI)) {
-          LLVMContext &Ctx = BI->getContext();
           MDNode *InlinedAt = BI->getDebugLoc().getInlinedAt(Ctx);
           DVI->setOperand(2, createInlinedVariable(DVI->getVariable(), 
                                                    InlinedAt, Ctx));
         }
       }
     }
   }
 }

test/DebugInfo/inline-debug-info-multiret.ll

 ; RUN: opt -inline -S < %s | FileCheck %s
 ;
 ; A hand-edited version of inline-debug-info.ll to test inlining of a
 ; function with multiple returns.
 ;
 ; Make sure the branch instructions created during inlining has a debug location,
 ; so the range of the inlined function is correct.
-; CHECK: br label %_Z4testi.exit, !dbg ![[MD:[0-9]+]]
-; CHECK: br label %_Z4testi.exit, !dbg ![[MD]]
-; CHECK: br label %invoke.cont, !dbg ![[MD]]
+; CHECK: br label %_Z4testi.exit, !dbg [[MD:![0-9]+]]
+; CHECK: br label %_Z4testi.exit, !dbg [[MD]]
+; CHECK: br label %invoke.cont, !dbg [[MD]]
 ; The branch instruction has the source location of line 9 and its inlined location
 ; has the source location of line 14.
-; CHECK: ![[INL:[0-9]+]] = metadata !{i32 14, i32 0, metadata {{.*}}, null}
-; CHECK: ![[MD]] = metadata !{i32 9, i32 0, metadata {{.*}}, metadata ![[INL]]}
+; CHECK: [[INL:![0-9]+]] = metadata !{i32 14, i32 0, metadata {{.*}}, null, metadata [[INL]]}
+; CHECK: [[MD]] = metadata !{i32 9, i32 0, metadata {{.*}}, metadata [[INL]]}
 
 ; ModuleID = 'test.cpp'
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-darwin12.0.0"
 
 @_ZTIi = external constant i8*
 @global_var = external global i32
 

test/DebugInfo/inline-debug-info.ll

 ; 18   }
 ; 19   global_var = 1;
 ; 20   return 0;
 ; 21 }
 
 ; CHECK: _Z4testi.exit:
 ; Make sure the branch instruction created during inlining has a debug location,
 ; so the range of the inlined function is correct.
-; CHECK: br label %invoke.cont, !dbg ![[MD:[0-9]+]]
+; CHECK: br label %invoke.cont, !dbg [[MD:![0-9]+]]
 ; The branch instruction has the source location of line 9 and its inlined location
 ; has the source location of line 14.
-; CHECK: ![[INL:[0-9]+]] = metadata !{i32 14, i32 0, metadata {{.*}}, null}
-; CHECK: ![[MD]] = metadata !{i32 9, i32 0, metadata {{.*}}, metadata ![[INL]]}
+; CHECK: [[INL:![0-9]*]] = metadata !{i32 14, i32 0, metadata {{.*}}, null, metadata [[INL]]}
+; CHECK: [[MD]] = metadata !{i32 9, i32 0, metadata {{.*}}, metadata [[INL]]}
 
 ; ModuleID = 'test.cpp'
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-darwin12.0.0"
 
 @_ZTIi = external constant i8*
 @global_var = external global i32
 

test/DebugInfo/inline-no-debug-info.ll

 ; CHECK: store i32 1, {{.*}}, !dbg [[A:!.*]]
 
 ; This instruction came from callee with a !dbg metadata.
 ; CHECK: store i32 2, {{.*}}, !dbg [[B:!.*]]
 
 ; The remaining instruction from the caller.
 ; CHECK: ret void, !dbg [[A]]
 
-; Debug location of the code in caller() and of the inlined code that did not
-; have any debug location before.
-; CHECK-DAG: [[A]] = metadata !{i32 4, i32 0, metadata !{{[01-9]+}}, null}
+; Debug location of the code in caller()
+; CHECK-DAG: [[A]] = metadata !{i32 4, i32 0, metadata !{{[0-9]+}}, null}
 
 ; Debug location of the inlined code.
-; CHECK-DAG: [[B]] = metadata !{i32 2, i32 0, metadata !{{[01-9]+}}, metadata [[A]]}
+; CHECK-DAG: [[B]] = metadata !{i32 2, i32 0, metadata !{{[0-9]+}}, metadata [[C:![0-9]+]]}
+
+; Location of the inlining (uniqued by call site, so this is distinct from [[A]] but describes the same location)
+; CHECK-DAG: [[C]] = metadata !{i32 4, i32 0, metadata !{{[0-9]+}}, null, metadata [[C]]}
 
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @a = common global i32 0, align 4
 @b = common global i32 0, align 4
 

test/Transforms/Inline/debug-info-duplicate-calls.ll

+; RUN: opt < %s -always-inline -S | FileCheck %s
+
+; Original input generated from clang -emit-llvm -S -c -mllvm -disable-llvm-optzns
+;
+; #define CALLS1 f2(); f2();
+; #define CALLS2 f4(); f4();
+; void f1();
+; inline __attribute__((always_inline)) void f2() {
+;   f1();
+; }
+; inline __attribute__((always_inline)) void f3() {
+;   CALLS1
+; }
+; inline __attribute__((always_inline)) void f4() {
+;   f3();
+; }
+; void f() {
+;   CALLS2
+; }
+
+; There should be unique locations for all 4 of these instructions, correctly
+; describing the inlining that has occurred, even in the face of duplicate call
+; site locations.
+
+; The nomenclature used for the tags here is <function name>[cs<number>] where
+; 'cs' is an abbreviation for 'call site' and the number indicates which call
+; site from within the named function this is. (so, given the above inlining, we
+; should have 4 calls to 'f1', two from the first call to f4 and two from the
+; second call to f4)
+
+; CHECK: call void @_Z2f1v(), !dbg [[fcs1_f4_f3cs1_f2:![0-9]+]]
+; CHECK: call void @_Z2f1v(), !dbg [[fcs1_f4_f3cs2_f2:![0-9]+]]
+; CHECK: call void @_Z2f1v(), !dbg [[fcs2_f4_f3cs1_f2:![0-9]+]]
+; CHECK: call void @_Z2f1v(), !dbg [[fcs2_f4_f3cs2_f2:![0-9]+]]
+
+; CHECK-DAG: [[F:![0-9]+]] = {{.*}} ; [ DW_TAG_subprogram ] {{.*}} [f]
+; CHECK-DAG: [[F2:![0-9]+]] = {{.*}} ; [ DW_TAG_subprogram ] {{.*}} [f2]
+; CHECK-DAG: [[F3:![0-9]+]] = {{.*}} ; [ DW_TAG_subprogram ] {{.*}} [f3]
+; CHECK-DAG: [[F4:![0-9]+]] = {{.*}} ; [ DW_TAG_subprogram ] {{.*}} [f4]
+
+; CHECK: [[fcs1_f4_f3cs1_f2]] = {{.*}}, metadata [[F2]], metadata [[fcs1_f4_f3cs1:![0-9]+]]}
+; CHECK: [[fcs1_f4_f3cs1]] = {{.*}}, metadata [[F3]], metadata [[fcs1_f4:![0-9]+]], metadata [[fcs1_f4_f3cs1]]}
+; CHECK: [[fcs1_f4]] = {{.*}}, metadata [[F4]], metadata [[fcs1:![0-9]+]], metadata [[fcs1_f4]]}
+; CHECK: [[fcs1]] = {{.*}}, metadata [[F]], null, metadata [[fcs1]]}
+; CHECK: [[fcs1_f4_f3cs2_f2]] = {{.*}}, metadata [[F2]], metadata [[fcs1_f4_f3cs2:![0-9]+]]}
+; CHECK: [[fcs1_f4_f3cs2]] = {{.*}}, metadata [[F3]], metadata [[fcs1_f4]], metadata [[fcs1_f4_f3cs2]]}
+
+; CHECK: [[fcs2_f4_f3cs1_f2]] = {{.*}}, metadata [[F2]], metadata [[fcs2_f4_f3cs1:![0-9]+]]}
+; CHECK: [[fcs2_f4_f3cs1]] = {{.*}}, metadata [[F3]], metadata [[fcs2_f4:![0-9]+]], metadata [[fcs2_f4_f3cs1]]}
+; CHECK: [[fcs2_f4]] = {{.*}}, metadata [[F4]], metadata [[fcs2:![0-9]+]], metadata [[fcs2_f4]]}
+; CHECK: [[fcs2]] = {{.*}}, metadata [[F]], null, metadata [[fcs2]]}
+; CHECK: [[fcs2_f4_f3cs2_f2]] = {{.*}}, metadata [[F2]], metadata [[fcs2_f4_f3cs2:![0-9]+]]}
+; CHECK: [[fcs2_f4_f3cs2]] = {{.*}}, metadata [[F3]], metadata [[fcs2_f4]], metadata [[fcs2_f4_f3cs2]]}
+
+; Function Attrs: uwtable
+define void @_Z1fv() #0 {
+entry:
+  call void @_Z2f4v(), !dbg !14
+  call void @_Z2f4v(), !dbg !14
+  ret void, !dbg !15
+}
+
+; Function Attrs: alwaysinline inlinehint uwtable
+define linkonce_odr void @_Z2f4v() #1 {
+entry:
+  call void @_Z2f3v(), !dbg !16
+  ret void, !dbg !17
+}
+
+; Function Attrs: alwaysinline inlinehint uwtable
+define linkonce_odr void @_Z2f3v() #1 {
+entry:
+  call void @_Z2f2v(), !dbg !18
+  call void @_Z2f2v(), !dbg !18
+  ret void, !dbg !19
+}
+
+; Function Attrs: alwaysinline inlinehint uwtable
+define linkonce_odr void @_Z2f2v() #1 {
+entry:
+  call void @_Z2f1v(), !dbg !20
+  ret void, !dbg !21
+}
+
+declare void @_Z2f1v() #2
+
+attributes #0 = { uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { alwaysinline inlinehint uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!11, !12}
+!llvm.ident = !{!13}
+
+!0 = metadata !{metadata !"0x11\004\00clang version 3.6.0 (trunk 222656) (llvm/trunk 222658)\000\00\000\00\001", metadata !1, metadata !2, metadata !2, metadata !3, metadata !2, metadata !2} ; [ DW_TAG_compile_unit ] [/tmp/dbginfo/debug-info-duplicate-calls.cpp] [DW_LANG_C_plus_plus]
+!1 = metadata !{metadata !"debug-info-duplicate-calls.cpp", metadata !"/tmp/dbginfo"}
+!2 = metadata !{}
+!3 = metadata !{metadata !4, metadata !8, metadata !9, metadata !10}
+!4 = metadata !{metadata !"0x2e\00f\00f\00_Z1fv\0013\000\001\000\000\00256\000\0013", metadata !1, metadata !5, metadata !6, null, void ()* @_Z1fv, null, null, metadata !2} ; [ DW_TAG_subprogram ] [line 13] [def] [f]
+!5 = metadata !{metadata !"0x29", metadata !1}    ; [ DW_TAG_file_type ] [/tmp/dbginfo/debug-info-duplicate-calls.cpp]
+!6 = metadata !{metadata !"0x15\00\000\000\000\000\000\000", null, null, null, metadata !7, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
+!7 = metadata !{null}
+!8 = metadata !{metadata !"0x2e\00f4\00f4\00_Z2f4v\0010\000\001\000\000\00256\000\0010", metadata !1, metadata !5, metadata !6, null, void ()* @_Z2f4v, null, null, metadata !2} ; [ DW_TAG_subprogram ] [line 10] [def] [f4]
+!9 = metadata !{metadata !"0x2e\00f3\00f3\00_Z2f3v\007\000\001\000\000\00256\000\007", metadata !1, metadata !5, metadata !6, null, void ()* @_Z2f3v, null, null, metadata !2} ; [ DW_TAG_subprogram ] [line 7] [def] [f3]
+!10 = metadata !{metadata !"0x2e\00f2\00f2\00_Z2f2v\004\000\001\000\000\00256\000\004", metadata !1, metadata !5, metadata !6, null, void ()* @_Z2f2v, null, null, metadata !2} ; [ DW_TAG_subprogram ] [line 4] [def] [f2]
+!11 = metadata !{i32 2, metadata !"Dwarf Version", i32 4}
+!12 = metadata !{i32 2, metadata !"Debug Info Version", i32 2}
+!13 = metadata !{metadata !"clang version 3.6.0 (trunk 222656) (llvm/trunk 222658)"}
+!14 = metadata !{i32 14, i32 3, metadata !4, null}
+!15 = metadata !{i32 15, i32 1, metadata !4, null}
+!16 = metadata !{i32 11, i32 3, metadata !8, null}
+!17 = metadata !{i32 12, i32 1, metadata !8, null}
+!18 = metadata !{i32 8, i32 3, metadata !9, null}
+!19 = metadata !{i32 9, i32 1, metadata !9, null}
+!20 = metadata !{i32 5, i32 3, metadata !10, null}
+!21 = metadata !{i32 6, i32 1, metadata !10, null}

test/Transforms/Inline/debug-invoke.ll

 ; RUN: opt < %s -always-inline -S | FileCheck %s
 
 ; Test that the debug location is preserved when rewriting an inlined call as an invoke
 
 ; CHECK: invoke void @test()
 ; CHECK-NEXT: to label {{.*}} unwind label {{.*}}, !dbg [[INL_LOC:!.*]]
 ; CHECK: [[EMPTY:.*]] = metadata !{}
 ; CHECK: [[INL_LOC]] = metadata !{i32 1, i32 0, metadata [[EMPTY]], metadata [[INL_AT:.*]]}
-; CHECK: [[INL_AT]] = metadata !{i32 2, i32 0, metadata [[EMPTY]], null}
+; CHECK: [[INL_AT]] = metadata !{i32 2, i32 0, metadata [[EMPTY]], null, metadata [[INL_AT]]}
 
 declare void @test()
 declare i32 @__gxx_personality_v0(...)
 
 attributes #0 = { alwaysinline }
 define void @inl() #0 {
   call void @test(), !dbg !3
   ret void