Only insert debug nodes after a phi if there is a valid insertion point

[keith.walker.arm]

Summary

When adding debug information to a lowered phi node in mem2reg
check that we have a valid insertion point after the phi for adding
the debug information.

This change addresses the issue in pr30468 where a lowered phi was
added before a catchswitch and no debug information should be added
after the phi in this case.

[hans]

I'm not familiar with this code. I suppose it boils down to whether it's OK that ConvertDebugDeclareToDebugValue() is now best-effort instead of always inserting the dbg intrinsic. If that's the case, then lgtm.

[aprantl]

Given our current restrictions for the catchswitch instruction this change is LGTM.
An alternative would be to relax the requirement that a catchswitch instruction is the getFirstNonPHIOrDbg() rather than getFirstNonPHI() instruction in the block.

[keith.walker.arm]

I believe this change is good as a general back-stop check.

I will look into whether using getFirstNonPHIOrDbg() would allow us to add debug data to a catchswitch as a separate improvement.

[majnemer]

LGTM too.

[rnk]

Did you forget to add the regression test for this?

[rnk]

I was going to add some comments, but I implemented them instead in rL282521. PTAL

lib/Transforms/Utils/Local.cpp

void llvm::ConvertDebugDeclareToDebugValue(DbgDeclareInst *DDI,
                                            PHINode *APN, DIBuilder &Builder) {
   auto *DIVar = DDI->getVariable();
   auto *DIExpr = DDI->getExpression();
   assert(DIVar && "Missing variable");
 
   if (PhiHasDebugValue(DIVar, DIExpr, APN))
     return;
 
+  auto BB = APN->getParent();
+  auto InsertionPt = BB->getFirstInsertionPt();
+  if (InsertionPt != BB->end()) {
     Instruction *DbgValue = Builder.insertDbgValueIntrinsic(
         APN, 0, DIVar, DIExpr, DDI->getDebugLoc(), (Instruction *)nullptr);
-  DbgValue->insertBefore(&*APN->getParent()->getFirstInsertionPt());
+    DbgValue->insertBefore(&*InsertionPt);
+  }
 }
 
 /// Determine whether this alloca is either a VLA or an array.
 static bool isArray(AllocaInst *AI) {
   return AI->isArrayAllocation() ||
     AI->getType()->getElementType()->isArrayTy();
 }
 

test/Transforms/Mem2Reg/PhiCatchSwitchAndDebug.ll

+; Do not generate debug information for a phi node before a catchswitch
+; Reported as pr30468
+
+; RUN: opt -mem2reg < %s -S | FileCheck %s
+; RUN: opt -passes=mem2reg < %s -S | FileCheck %s
+
+; Original C++ source for the reproducer:
+;
+; namespace win {
+; class PEImage {
+; public:
+;   PEImage(int);
+;   void GetNTHeaders();
+; };
+; }
+; int *AnsiToUnicode(void *) {
+;   int module;
+;   int *out_name;
+;   __try {
+;     win::PEImage pe(module);
+;     char *name;
+;     out_name = AnsiToUnicode(name);
+;     pe.GetNTHeaders();
+;   } __except (1) {
+;   }
+;   return out_name;
+; }
+;
+; with the compile options:
+; clang -S -g --target=x86_64-pc-windows-msvc19.0.0 -Oz -fms-compatibility -emit-llvm
+
+%"class.win::PEImage" = type { i8 }
+
+; Function Attrs: uwtable
+define i32* @"\01?AnsiToUnicode@@YAPEAHPEAX@Z"(i8*) #0 personality i8* bitcast (i32 (...)* @__C_specific_handler to i8*) !dbg !7 {
+entry:
+  %.addr = alloca i8*, align 8
+  %module = alloca i32, align 4
+  %out_name = alloca i32*, align 8
+  %__exception_code = alloca i32, align 4
+  %pe = alloca %"class.win::PEImage", align 1
+  %name = alloca i8*, align 8
+  store i8* %0, i8** %.addr, align 8
+  call void @llvm.dbg.declare(metadata i8** %.addr, metadata !13, metadata !14), !dbg !15
+  call void @llvm.dbg.declare(metadata i32* %module, metadata !16, metadata !14), !dbg !17
+  call void @llvm.dbg.declare(metadata i32** %out_name, metadata !18, metadata !14), !dbg !19
+  call void @llvm.dbg.declare(metadata %"class.win::PEImage"* %pe, metadata !20, metadata !14), !dbg !32
+  %1 = load i32, i32* %module, align 4, !dbg !33
+  %call = invoke %"class.win::PEImage"* @"\01??0PEImage@win@@QEAA@H@Z"(%"class.win::PEImage"* %pe, i32 %1) #3
+          to label %invoke.cont unwind label %catch.dispatch, !dbg !32
+
+invoke.cont:                                      ; preds = %entry
+  call void @llvm.dbg.declare(metadata i8** %name, metadata !34, metadata !14), !dbg !37
+  %2 = load i8*, i8** %name, align 8, !dbg !38
+  %call2 = invoke i32* @"\01?AnsiToUnicode@@YAPEAHPEAX@Z"(i8* %2) #3
+          to label %invoke.cont1 unwind label %catch.dispatch, !dbg !39
+
+invoke.cont1:                                     ; preds = %invoke.cont
+  store i32* %call2, i32** %out_name, align 8, !dbg !40
+  invoke void @"\01?GetNTHeaders@PEImage@win@@QEAAXXZ"(%"class.win::PEImage"* %pe) #3
+          to label %__try.cont unwind label %catch.dispatch, !dbg !42
+
+catch.dispatch:                                   ; preds = %invoke.cont1, %invoke.cont, %entry
+  %3 = catchswitch within none [label %__except] unwind to caller, !dbg !43
+
+; Check that the catchswitch is immediately after the phi and no debug metatdata node
+; has been inserted between them
+;
+; CHECK: %out_name.0 = phi i32* [ %call2, %invoke.cont1 ], [ undef, %invoke.cont ], [ undef, %entry ]
+; CHECK-NEXT: %1 = catchswitch within none [label %__except] unwind to caller
+
+__except:                                         ; preds = %catch.dispatch
+  %4 = catchpad within %3 [i8* null], !dbg !44
+  catchret from %4 to label %__except4, !dbg !44
+
+__except4:                                        ; preds = %__except
+  %5 = call i32 @llvm.eh.exceptioncode(token %4), !dbg !43
+  store i32 %5, i32* %__exception_code, align 4, !dbg !45
+  br label %__try.cont, !dbg !47
+
+__try.cont:                                       ; preds = %__except4, %invoke.cont1
+  %6 = load i32*, i32** %out_name, align 8, !dbg !49
+  ret i32* %6, !dbg !50
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+declare %"class.win::PEImage"* @"\01??0PEImage@win@@QEAA@H@Z"(%"class.win::PEImage"* returned, i32) unnamed_addr #2
+
+declare i32 @__C_specific_handler(...)
+
+declare void @"\01?GetNTHeaders@PEImage@win@@QEAAXXZ"(%"class.win::PEImage"*) #2
+
+; Function Attrs: nounwind readnone
+declare i32 @llvm.eh.exceptioncode(token) #1
+
+attributes #0 = { uwtable }
+attributes #1 = { nounwind readnone }
+attributes #3 = { noinline }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "reproducer.cpp", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"PIC Level", i32 2}
+!6 = !{!"clang"}
+!7 = distinct !DISubprogram(name: "AnsiToUnicode", linkageName: "\01?AnsiToUnicode@@YAPEAHPEAX@Z", scope: !1, file: !1, line: 8, type: !8, isLocal: false, isDefinition: true, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !12}
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64, align: 64)
+!11 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64, align: 64)
+!13 = !DILocalVariable(arg: 1, scope: !7, file: !1, line: 8, type: !12)
+!14 = !DIExpression()
+!15 = !DILocation(line: 8, column: 26, scope: !7)
+!16 = !DILocalVariable(name: "module", scope: !7, file: !1, line: 9, type: !11)
+!17 = !DILocation(line: 9, column: 7, scope: !7)
+!18 = !DILocalVariable(name: "out_name", scope: !7, file: !1, line: 10, type: !10)
+!19 = !DILocation(line: 10, column: 8, scope: !7)
+!20 = !DILocalVariable(name: "pe", scope: !21, file: !1, line: 12, type: !22)
+!21 = distinct !DILexicalBlock(scope: !7, file: !1, line: 11, column: 9)
+!22 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "PEImage", scope: !23, file: !1, line: 2, size: 8, align: 8, elements: !24, identifier: ".?AVPEImage@win@@")
+!23 = !DINamespace(name: "win", scope: null, file: !1, line: 1)
+!24 = !{!25, !29}
+!25 = !DISubprogram(name: "PEImage", scope: !22, file: !1, line: 4, type: !26, isLocal: false, isDefinition: false, scopeLine: 4, flags: DIFlagPublic | DIFlagPrototyped, isOptimized: false)
+!26 = !DISubroutineType(types: !27)
+!27 = !{null, !28, !11}
+!28 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !22, size: 64, align: 64, flags: DIFlagArtificial | DIFlagObjectPointer)
+!29 = !DISubprogram(name: "GetNTHeaders", linkageName: "\01?GetNTHeaders@PEImage@win@@QEAAXXZ", scope: !22, file: !1, line: 5, type: !30, isLocal: false, isDefinition: false, scopeLine: 5, flags: DIFlagPublic | DIFlagPrototyped, isOptimized: false)
+!30 = !DISubroutineType(types: !31)
+!31 = !{null, !28}
+!32 = !DILocation(line: 12, column: 18, scope: !21)
+!33 = !DILocation(line: 12, column: 21, scope: !21)
+!34 = !DILocalVariable(name: "name", scope: !21, file: !1, line: 13, type: !35)
+!35 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !36, size: 64, align: 64)
+!36 = !DIBasicType(name: "char", size: 8, align: 8, encoding: DW_ATE_signed_char)
+!37 = !DILocation(line: 13, column: 11, scope: !21)
+!38 = !DILocation(line: 14, column: 30, scope: !21)
+!39 = !DILocation(line: 14, column: 16, scope: !21)
+!40 = !DILocation(line: 14, column: 14, scope: !41)
+!41 = !DILexicalBlockFile(scope: !21, file: !1, discriminator: 1)
+!42 = !DILocation(line: 15, column: 8, scope: !21)
+!43 = !DILocation(line: 16, column: 3, scope: !21)
+!44 = !DILocation(line: 16, column: 3, scope: !41)
+!45 = !DILocation(line: 16, column: 3, scope: !46)
+!46 = !DILexicalBlockFile(scope: !21, file: !1, discriminator: 2)
+!47 = !DILocation(line: 17, column: 3, scope: !48)
+!48 = distinct !DILexicalBlock(scope: !7, file: !1, line: 16, column: 18)
+!49 = !DILocation(line: 18, column: 10, scope: !7)
+!50 = !DILocation(line: 18, column: 3, scope: !7)