Cloning: Fix debug info cloning

[GorNishanov]

Summary

I believe https://reviews.llvm.org/rL302576 introduced two bugs:

1. it produces duplicate distinct variables for every: dbg.value describing the same variable. To fix the problme I switched form getDistinct() to get() in DebugLoc.cpp: auto reparentVar = [&](DILocalVariable *Var) { return DILocalVariable::getDistinct(

2. It passes NewFunction plain name as a linkagename parameter to Subprogram constructor. Breaks assert in:
   
   || DeclLinkageName.empty()) || LinkageName == DeclLinkageName) && "decl has a linkage name and it is different"' failed.

#9 0x00007f5010261b75 llvm::DwarfUnit::applySubprogramDefinitionAttributes(llvm::DISubprogram const*, llvm::DIE&) /home/gor/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1173:3
#
(Edit: reproducer added)

Here how https://reviews.llvm.org/rL302576 broke coroutine debug info.
Coroutine body of the original function is split into several parts by cloning and removing unneeded code.
All parts describe the original function and variables present in the original function.

For a simple case, prior to Split, original function has these two blocks:

PostSpill:                                        ; preds = %AllocaSpillBB
  call void @llvm.dbg.value(metadata i32 %x, i64 0, metadata !14, metadata !15), !dbg !13
  store i32 %x, i32* %x.addr, align 4
  ...
and

sw.epilog:                                        ; preds = %sw.bb
  %x.addr.reload.addr = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4, !dbg !20
  %4 = load i32, i32* %x.addr.reload.addr, align 4, !dbg !20
  call void @llvm.dbg.value(metadata i32 %4, i64 0, metadata !14, metadata !15), !dbg !13

!14 = !DILocalVariable(name: "x", arg: 1, scope: !6, file: !7, line: 55, type: !11)

Note that in two blocks different expression represent the same original user variable X.

Before rL302576, for every cloned function there was exactly one cloned DILocalVariable(name: "x" as in:

define i8* @f(i32 %x) #0 !dbg !6 {
  ...
!6 = distinct !DISubprogram(name: "f", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55, flags: DIFlagPrototyped, 
...
!14 = !DILocalVariable(name: "x", arg: 1, scope: !6, file: !7, line: 55, type: !11)

define internal fastcc void @f.resume(%f.Frame* %FramePtr) #0 !dbg !25 {
...
!25 = distinct !DISubprogram(name: "f", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
!28 = !DILocalVariable(name: "x", arg: 1, scope: !25, file: !7, line: 55, type: !11)

After rL302576, for every cloned function there were as many DILocalVariable(name: "x" as there were "call void @llvm.dbg.value" for that variable.
This was causing asserts in VerifyDebugInfo and AssemblyPrinter.

Example:

!27 = distinct !DISubprogram(name: "f", linkageName: "f.resume", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55, 
!29 = distinct !DILocalVariable(name: "x", arg: 1, scope: !27, file: !7, line: 55, type: !11)
!39 = distinct !DILocalVariable(name: "x", arg: 1, scope: !27, file: !7, line: 55, type: !11)
!41 = distinct !DILocalVariable(name: "x", arg: 1, scope: !27, file: !7, line: 55, type: !11)

Second problem:

Prior to rL302576, all clones were described by DISubprogram referring to original function.

define i8* @f(i32 %x) #0 !dbg !6 {
...
!6 = distinct !DISubprogram(name: "f", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55, flags: DIFlagPrototyped, 

define internal fastcc void @f.resume(%f.Frame* %FramePtr) #0 !dbg !25 {
...
!25 = distinct !DISubprogram(name: "f", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55, flags: DIFlagPrototyped,

After rL302576, DISubprogram for clones is of two minds, plain name refers to the original name, linkageName refers to plain name of the clone.

!27 = distinct !DISubprogram(name: "f", linkageName: "f.resume", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55,

I think the assumption in AsmPrinter is that both name and linkageName should refer to the same entity. It asserts here when they are not:

 || DeclLinkageName.empty()) || LinkageName == DeclLinkageName) && "decl has a linkage name and it is different"' failed.
#9 0x00007f5010261b75 llvm::DwarfUnit::applySubprogramDefinitionAttributes(llvm::DISubprogram const*, llvm::DIE&) /home/gor/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1173:3

After this fix, behavior (with respect to coroutines) reverts to exactly as it was before and therefore making them debuggable again, or even more importantly, compilable, with "-g"

[dblaikie]

The assertion is that, in the presence of a separate declaration (eg: a member function that is a coroutine - so the member variable gets a declaration (this is about the only time LLVM has/uses a separate declaration for a function/subprogram in the debug info) and models an out of line definition (even if the definition is in-line)) that if there is a linkage name for the declaration, then it's the same as the linkage name for the definition.

This seems pretty wrong for coroutines where the debug info looks like it's going to have 3-4 functions, with different mangled names, all that claim to be an implementation of the same declaration? Their mangled names won't match the declaration anyway...

I think maybe, it'd be better to drop the linkage name when cloning. (or use the linkage name of the clone - and change/relax the assertion) It won't be better in the end - the debug info would still have multiple definitions referring to the same declaration and if the declaration has a linkage name it'll be right for at most one of those functions and wrong for the rest. But at least the IR won't be /as/ wrong... ish.

[GorNishanov]

In D33614#766379, @dblaikie wrote:

The assertion is that, in the presence of a separate declaration (eg: a member function that is a coroutine - so the member variable gets a declaration (this is about the only time LLVM has/uses a separate declaration for a function/subprogram in the debug info) and models an out of line definition (even if the definition is in-line)) that if there is a linkage name for the declaration, then it's the same as the linkage name for the definition.

This seems pretty wrong for coroutines where the debug info looks like it's going to have 3-4 functions, with different mangled names, all that claim to be an implementation of the same declaration? Their mangled names won't match the declaration anyway...

I think maybe, it'd be better to drop the linkage name when cloning. (or use the linkage name of the clone - and change/relax the assertion) It won't be better in the end - the debug info would still have multiple definitions referring to the same declaration and if the declaration has a linkage name it'll be right for at most one of those functions and wrong for the rest. But at least the IR won't be /as/ wrong... ish.

Something like this?

Replace:

SP->getName(), NewFunc->getName()  (rL302576 way)

or

SP->getName(), SP->getLinkageName() (This patch way)

with

NewFunc->getName(), NewFunc->getName(),  (dblakie way?)

Just tested:

dblakie way triggers the same assert as rL302576 way.
I am keeping "this patch way" for know

[GorNishanov]

Test updated to catch breakage if it happens again. Preparing to land to unblock coroutine debugging (or compiling with -g).

[dblaikie]

(once you've sent something for review, please wait until it's approved - generally the assumption/concensus is that if a developer asks for review, it's because it's needed - submitting then without review/sign off indicates that maybe the required review didn't happen, which isn't good)

lib/IR/DebugLoc.cpp

170 ↗

(On Diff #100517)

Is there a reason this can't be distinct anymore?

You mentioned in the description: "I added caching, but, it is not sufficient, since dbg.value may refer to various expressions representing the same user var, thus, switching form getDistinct to get is important."

But I don't follow quite what you're saying there - could you explain it in more detail/in another way?

What happens without changing this to 'get'?

[GorNishanov]

In D33614#766434, @dblaikie wrote:

(once you've sent something for review, please wait until it's approved - generally the assumption/concensus is that if a developer asks for review, it's because it's needed - submitting then without review/sign off indicates that maybe the required review didn't happen, which isn't good)

Sure thing. I'll wait for LGTM.

[GorNishanov]

lib/IR/DebugLoc.cpp

170 ↗

(On Diff #100517)

It can still creates duplicate DILocalVariables. I have it in the very large test case, I can see if I can reduce it to something manangable. Thank you for looking at this!

[GorNishanov]

lib/IR/DebugLoc.cpp

170 ↗

(On Diff #100517)

Hah. Cache by itself does not work since it is cleared for every basic block.
I am not sure what are the implications of moving it outside of CloneBasicBlock.

I would keep the cache where it is, and keep get as opposed to getDistinct to collapse duplicate LocalVariables to the same one.

170 ↗

(On Diff #100517)

Good to go?

[GorNishanov]

lib/IR/DebugLoc.cpp

170 ↗

(On Diff #100517)

Alternative, since the cache scope is just one basic block, I can remove the caching from auto reparentVar altogether, so the fix in lib/IR/DebugLoc.cpp will be simply:

s/getDistinct/get

in

auto reparentVar = [&](DILocalVariable *Var) {
    return DILocalVariable::getDistinct(

[GorNishanov]

Simplified the fix in IR/DebugLoc.cpp to s/getDistinct/get.
Granted mountains of wealth token for those kind souls who would review this change.

[dblaikie]

Looks good enough to unblock coroutine work.

Adrian, when you get a chance, could you look this over & make any better/longer-term fixes?

My rough thoughts are:
DILocalVariables aren't distinct when clang creates them, so probably fine not to create them distinct here. But perhaps that decision, globally, should be revisited (& in that case add a function-wide caching for this cloning step).

Linkage name - While preserving it ensures that it won't conflict with the DISubprogram representing the function declaration, if there is one, it seems wrong because this function now has a potentially different linkage name. I'd thought "if there is a declaration, remove the linkage name", but maybe that's not even right - may be that the right thing is to actually have a different linkage name from the one on the declaration? Not sure.

llvm/trunk/lib/IR/DebugLoc.cpp

void DebugLoc::reparentDebugInfo(Instruction &I, DISubprogram *OrigSP,
   I.setDebugLoc(
       DebugLoc::get(DL.getLine(), DL.getCol(),
                     reparentScope(Ctx, DL->getScope(), OrigSP, NewSP, Cache),
                     DebugLoc::appendInlinedAt(DL, InlinedAt, Ctx, Cache,
                                               ReplaceLastInlinedAt)));
 
   // Fix up debug variables to point to NewSP.
   auto reparentVar = [&](DILocalVariable *Var) {
-    return DILocalVariable::getDistinct(
+    return DILocalVariable::get(
         Ctx,
         cast<DILocalScope>(
             reparentScope(Ctx, Var->getScope(), OrigSP, NewSP, Cache)),
         Var->getName(), Var->getFile(), Var->getLine(), Var->getType(),
         Var->getArg(), Var->getFlags(), Var->getAlignInBits());
   };
   if (auto *DbgValue = dyn_cast<DbgValueInst>(&I)) {
     auto *Var = DbgValue->getVariable();

llvm/trunk/lib/Transforms/Coroutines/CoroSplit.cpp

static Function *createClone(Function &F, Twine Suffix, coro::Shape &Shape,
   // Replace all args with undefs. The buildCoroutineFrame algorithm already
   // rewritten access to the args that occurs after suspend points with loads
   // and stores to/from the coroutine frame.
   for (Argument &A : F.args())
     VMap[&A] = UndefValue::get(A.getType());
 
   SmallVector<ReturnInst *, 4> Returns;
 
-  if (DISubprogram *SP = F.getSubprogram()) {
-    // If we have debug info, add mapping for the metadata nodes that should not
-    // be cloned by CloneFunctionInfo.
-    auto &MD = VMap.MD();
-    MD[SP->getUnit()].reset(SP->getUnit());
-    MD[SP->getType()].reset(SP->getType());
-    MD[SP->getFile()].reset(SP->getFile());
-  }
-  CloneFunctionInto(NewF, &F, VMap, /*ModuleLevelChanges=*/true, Returns);
+  CloneFunctionInto(NewF, &F, VMap, /*ModuleLevelChanges=*/false, Returns);
 
   // Remove old returns.
   for (ReturnInst *Return : Returns)
     changeToUnreachable(Return, /*UseLLVMTrap=*/false);
 
   // Remove old return attributes.
   NewF->removeAttributes(
       AttributeList::ReturnIndex,

llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp

for (auto MD : MDs) {
     MDNode *NewMD;
     bool MustCloneSP =
         (MD.first == LLVMContext::MD_dbg && OldFunc->getParent() &&
          OldFunc->getParent() == NewFunc->getParent());
     if (MustCloneSP) {
       auto *SP = cast<DISubprogram>(MD.second);
       NewMD = DISubprogram::getDistinct(
           NewFunc->getContext(), SP->getScope(), SP->getName(),
-          NewFunc->getName(), SP->getFile(), SP->getLine(), SP->getType(),
+          SP->getLinkageName(), SP->getFile(), SP->getLine(), SP->getType(),
           SP->isLocalToUnit(), SP->isDefinition(), SP->getScopeLine(),
           SP->getContainingType(), SP->getVirtuality(), SP->getVirtualIndex(),
           SP->getThisAdjustment(), SP->getFlags(), SP->isOptimized(),
           SP->getUnit(), SP->getTemplateParams(), SP->getDeclaration(),
           SP->getVariables(), SP->getThrownTypes());
     } else
       NewMD =
           MapMetadata(MD.second, VMap,

llvm/trunk/test/Transforms/Coroutines/coro-debug.ll

+; Tests that debug information is sane after coro-split
+; RUN: opt < %s -coro-split -S | FileCheck %s
+
+source_filename = "simple-repro.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind
+define i8* @f(i32 %x) #0 !dbg !6 {
+entry:
+  %x.addr = alloca i32, align 4
+  %coro_hdl = alloca i8*, align 8
+  store i32 %x, i32* %x.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %x.addr, metadata !12, metadata !13), !dbg !14
+  call void @llvm.dbg.declare(metadata i8** %coro_hdl, metadata !15, metadata !13), !dbg !16
+  %0 = call token @llvm.coro.id(i32 0, i8* null, i8* bitcast (i8* (i32)* @f to i8*), i8* null), !dbg !16
+  %1 = call i64 @llvm.coro.size.i64(), !dbg !16
+  %call = call i8* @malloc(i64 %1), !dbg !16
+  %2 = call i8* @llvm.coro.begin(token %0, i8* %call) #7, !dbg !16
+  store i8* %2, i8** %coro_hdl, align 8, !dbg !16
+  %3 = call i8 @llvm.coro.suspend(token none, i1 false), !dbg !17
+  %conv = sext i8 %3 to i32, !dbg !17
+  call void @coro.devirt.trigger(i8* null)
+  switch i32 %conv, label %sw.default [
+    i32 0, label %sw.bb
+    i32 1, label %sw.bb1
+  ], !dbg !17
+
+sw.bb:                                            ; preds = %entry
+  br label %sw.epilog, !dbg !18
+
+sw.bb1:                                           ; preds = %entry
+  br label %coro_Cleanup, !dbg !18
+
+sw.default:                                       ; preds = %entry
+  br label %coro_Suspend, !dbg !18
+
+sw.epilog:                                        ; preds = %sw.bb
+  %4 = load i32, i32* %x.addr, align 4, !dbg !20
+  %add = add nsw i32 %4, 1, !dbg !21
+  store i32 %add, i32* %x.addr, align 4, !dbg !22
+  br label %coro_Cleanup, !dbg !23
+
+coro_Cleanup:                                     ; preds = %sw.epilog, %sw.bb1
+  %5 = load i8*, i8** %coro_hdl, align 8, !dbg !24
+  %6 = call i8* @llvm.coro.free(token %0, i8* %5), !dbg !24
+  call void @free(i8* %6), !dbg !24
+  br label %coro_Suspend, !dbg !24
+
+coro_Suspend:                                     ; preds = %coro_Cleanup, %sw.default
+  %7 = call i1 @llvm.coro.end(i8* null, i1 false) #7, !dbg !24
+  %8 = load i8*, i8** %coro_hdl, align 8, !dbg !24
+  ret i8* %8, !dbg !24
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: argmemonly nounwind readonly
+declare token @llvm.coro.id(i32, i8* readnone, i8* nocapture readonly, i8*) #2
+
+declare i8* @malloc(i64) #3
+
+; Function Attrs: nounwind readnone
+declare i64 @llvm.coro.size.i64() #4
+
+; Function Attrs: nounwind
+declare i8* @llvm.coro.begin(token, i8* writeonly) #5
+
+; Function Attrs: nounwind
+declare i8 @llvm.coro.suspend(token, i1) #5
+
+declare void @free(i8*) #3
+
+; Function Attrs: argmemonly nounwind readonly
+declare i8* @llvm.coro.free(token, i8* nocapture readonly) #2
+
+; Function Attrs: nounwind
+declare i1 @llvm.coro.end(i8*, i1) #5
+
+; Function Attrs: alwaysinline
+define private void @coro.devirt.trigger(i8*) #6 {
+entry:
+  ret void
+}
+
+; Function Attrs: argmemonly nounwind readonly
+declare i8* @llvm.coro.subfn.addr(i8* nocapture readonly, i8) #2
+
+attributes #0 = { noinline nounwind "coroutine.presplit"="1" "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone speculatable }
+attributes #2 = { argmemonly nounwind readonly }
+attributes #3 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #4 = { nounwind readnone }
+attributes #5 = { nounwind }
+attributes #6 = { alwaysinline }
+attributes #7 = { noduplicate }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 5.0.0 (http://llvm.org/git/clang.git 97b002238b11ff30d94d0516d6a0515db5725fd8) (http://llvm.org/git/llvm.git 0cb060ba567f1aa5b4b04e86665f88e4632b528a)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "<stdin>", directory: "C:\5CGitHub\5Cllvm\5Cbuild\5CDebug\5Cbin")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"clang version 5.0.0 (http://llvm.org/git/clang.git 97b002238b11ff30d94d0516d6a0515db5725fd8) (http://llvm.org/git/llvm.git 0cb060ba567f1aa5b4b04e86665f88e4632b528a)"}
+!6 = distinct !DISubprogram(name: "f", linkageName: "flink", scope: !7, file: !7, line: 55, type: !8, isLocal: false, isDefinition: true, scopeLine: 55, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!7 = !DIFile(filename: "simple-repro.c", directory: "C:\5CGitHub\5Cllvm\5Cbuild\5CDebug\5Cbin")
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !11}
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !DILocalVariable(name: "x", arg: 1, scope: !6, file: !7, line: 55, type: !11)
+!13 = !DIExpression()
+!14 = !DILocation(line: 55, column: 13, scope: !6)
+!15 = !DILocalVariable(name: "coro_hdl", scope: !6, file: !7, line: 56, type: !10)
+!16 = !DILocation(line: 56, column: 3, scope: !6)
+!17 = !DILocation(line: 58, column: 5, scope: !6)
+!18 = !DILocation(line: 58, column: 5, scope: !19)
+!19 = distinct !DILexicalBlock(scope: !6, file: !7, line: 58, column: 5)
+!20 = !DILocation(line: 59, column: 9, scope: !6)
+!21 = !DILocation(line: 59, column: 10, scope: !6)
+!22 = !DILocation(line: 59, column: 7, scope: !6)
+!23 = !DILocation(line: 59, column: 5, scope: !6)
+!24 = !DILocation(line: 62, column: 3, scope: !6)
+
+; CHECK: define i8* @f(i32 %x) #0 !dbg ![[ORIG:[0-9]+]]
+; CHECK: define internal fastcc void @f.resume(%f.Frame* %FramePtr) #0 !dbg ![[RESUME:[0-9]+]]
+; CHECK: define internal fastcc void @f.destroy(%f.Frame* %FramePtr) #0 !dbg ![[DESTROY:[0-9]+]]
+; CHECK: define internal fastcc void @f.cleanup(%f.Frame* %FramePtr) #0 !dbg ![[CLEANUP:[0-9]+]]
+
+; CHECK: ![[ORIG]] = distinct !DISubprogram(name: "f", linkageName: "flink"
+; CHECK: !DILocalVariable(name: "x", arg: 1, scope: ![[ORIG]]
+
+; CHECK: ![[RESUME]] = distinct !DISubprogram(name: "f", linkageName: "flink"
+; CHECK: !DILocalVariable(name: "x", arg: 1, scope: ![[RESUME]]
+
+; CHECK: ![[DESTROY]] = distinct !DISubprogram(name: "f", linkageName: "flink"
+
+; CHECK: ![[CLEANUP]] = distinct !DISubprogram(name: "f", linkageName: "flink"