[CodeExtractor] Erase debug intrinsics in outlined thunks

[vsk]

Summary

Variable updates within the outlined function are invisible to
debuggers. This could be improved by defining a DISubprogram for the
new function. For the moment, simply erase the debug intrinsics instead.

This fixes verifier failures about function-local metadata being used in
the wrong function, seen while testing the hot/cold splitting pass.

rdar://45142482

[aprantl]

What kind of DISubprogram do we assign to the outlined function now?

[vsk]

In D53267#1265513, @aprantl wrote:

What kind of DISubprogram do we assign to the outlined function now?

Currently we don't assign one.

[aprantl]

That's not great...

[sebpop]

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

[vsk]

In D53267#1265641, @sebpop wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

[tejohnson]

In D53267#1265669, @vsk wrote:

In D53267#1265641, @sebpop wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

[tejohnson]

In D53267#1265670, @tejohnson wrote:

In D53267#1265669, @vsk wrote:

In D53267#1265641, @sebpop wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

[vsk]

In D53267#1265680, @tejohnson wrote:

In D53267#1265670, @tejohnson wrote:

In D53267#1265669, @vsk wrote:

In D53267#1265641, @sebpop wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

Hm, I think that's the same failure mode I saw. A dbg.value from the original function was moved to the outlined function, but its ValueAsMetadata operand (the "function-local metadata") wasn't updated to point to an Argument in the outlined function. The error I'm referring to comes from Verifier::visitValueAsMetadata.

[tejohnson]

In D53267#1265681, @vsk wrote:

In D53267#1265680, @tejohnson wrote:

In D53267#1265670, @tejohnson wrote:

In D53267#1265669, @vsk wrote:

In D53267#1265641, @sebpop wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

Hm, I think that's the same failure mode I saw. A dbg.value from the original function was moved to the outlined function, but its ValueAsMetadata operand (the "function-local metadata") wasn't updated to point to an Argument in the outlined function.

Yeah, sounds like it.

The error I'm referring to comes from Verifier::visitValueAsMetadata.

Yep, I saw that one when building with hot cold splitting and -g. Originally the failure I saw was different, because it manifests differently when building with ThinLTO, where essentially the bitcode gets corrupted because the operand types don't match.

[vsk]

In D53267#1265690, @tejohnson wrote:

In D53267#1265681, @vsk wrote:

In D53267#1265680, @tejohnson wrote:

In D53267#1265670, @tejohnson wrote:

In D53267#1265669, @vsk wrote:

In D53267#1265641, @sebpop wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

Hm, I think that's the same failure mode I saw. A dbg.value from the original function was moved to the outlined function, but its ValueAsMetadata operand (the "function-local metadata") wasn't updated to point to an Argument in the outlined function.

Yeah, sounds like it.

The error I'm referring to comes from Verifier::visitValueAsMetadata.

Yep, I saw that one when building with hot cold splitting and -g. Originally the failure I saw was different, because it manifests differently when building with ThinLTO, where essentially the bitcode gets corrupted because the operand types don't match.

I think I know what you're talking about. I saw something similar while building an internal framework with -flto=thin: with the verifier disabled, it manifested as an assertion failure in the bitcode writer's ValueEnumerator. But the underlying issue is the same as the one diagnosed by the verifier.

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

for (BasicBlock::iterator I = Succs[i]->begin(); isa<PHINode>(I); ++I) {
             // There were multiple entries in the PHI for this block, now there
             // is only one, so remove the duplicated entries.
             PN->removeIncomingValue(i, false);
             --i; --e;
           }
         }
     }
 
+  // Erase debug info intrinsics. Variable updates within the new function are
+  // invisible to debuggers. This could be improved by defining a DISubprogram
+  // for the new function.
+  for (BasicBlock &BB : *newFunction) {
+    auto BlockIt = BB.begin();
+    while (BlockIt != BB.end()) {
+      Instruction *Inst = &*BlockIt;
+      ++BlockIt;
+      if (isa<DbgInfoIntrinsic>(Inst))
+        Inst->eraseFromParent();
+    }
+  }
+
   LLVM_DEBUG(if (verifyFunction(*newFunction))
                  report_fatal_error("verifyFunction failed!"));
   return newFunction;
 }

llvm/trunk/test/Transforms/HotColdSplit/split-out-dbg-val-of-arg.ll

+; RUN: opt -hotcoldsplit -S < %s | FileCheck %s
+
+; CHECK-LABEL: define {{.*}}@foo_if.end
+; CHECK-NOT: llvm.dbg.value
+
+define void @foo(i32 %arg1) !dbg !6 {
+entry:
+  %var = add i32 0, 0, !dbg !11
+  br i1 undef, label %if.then, label %if.end, !dbg !12
+
+if.then:                                          ; preds = %entry
+  unreachable, !dbg !13
+
+if.end:                                           ; preds = %entry
+  call void @llvm.dbg.value(metadata i32 %arg1, metadata !9, metadata !DIExpression()), !dbg !11
+  br label %if.then12, !dbg !14
+
+if.then12:                                        ; preds = %if.end
+  br label %cleanup40, !dbg !15
+
+cleanup40:                                        ; preds = %if.then12
+  br label %return, !dbg !16
+
+return:                                           ; preds = %cleanup40
+  ret void, !dbg !17
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!3, !4}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "<stdin>", directory: "/")
+!2 = !{}
+!3 = !{i32 7}
+!4 = !{i32 1}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, isLocal: false, isDefinition: true, scopeLine: 1, isOptimized: true, unit: !0, retainedNodes: !8)
+!7 = !DISubroutineType(types: !2)
+!8 = !{!9}
+!9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
+!10 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!11 = !DILocation(line: 1, column: 1, scope: !6)
+!12 = !DILocation(line: 2, column: 1, scope: !6)
+!13 = !DILocation(line: 3, column: 1, scope: !6)
+!14 = !DILocation(line: 4, column: 1, scope: !6)
+!15 = !DILocation(line: 5, column: 1, scope: !6)
+!16 = !DILocation(line: 6, column: 1, scope: !6)
+!17 = !DILocation(line: 7, column: 1, scope: !6)