[llvm] Add DW_CC_nocall to function debug metadata when either return values or arguments are removed

[RamNalamothu]

Summary

Adding the DW_CC_nocall calling convention to the function debug metadata is needed when either the return values or the arguments of a function are removed as this helps in informing debugger that it may not be safe to call this function or try to interpret the return value.
This translates to setting DW_AT_calling_convention with DW_CC_nocall for appropriate DWARF DIEs.

The DWARF5 spec (section 3.3.1.1 Calling Convention Information) says:

If the DW_AT_calling_convention attribute is not present, or its value is the constant DW_CC_normal, then the subroutine may be safely called by obeying the standard calling conventions of the target architecture. If the value of the calling convention attribute is the constant DW_CC_nocall, the subroutine does not obey standard calling conventions, and it may not be safe for the debugger to call this subroutine.

[dblaikie]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1080–1081

I think this FIXME is correct, and we should probably just check if the type matches or not. Happy for that to go into this change rather than being a separate one - and if someone figures out some needle-threading nuance where the types might mismatch but still be call-compatible, they can add that back in later.

llvm/test/Transforms/DeadArgElim/2010-04-30-DbgInfo.ll

4–6

Generally we don't use 'grep' for tests in LLVM (very old pre-FileCheck tests use this sometimes, but we try to avoid doing it in new tests)

Please update these to use FileCheck (& you can probably leave the RUN line as-is, I think - without needing to introduce the %t and cat usage)

[RamNalamothu]

Address review feedback.

[RamNalamothu]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1080–1081

Done.

llvm/test/Transforms/DeadArgElim/2010-04-30-DbgInfo.ll

4–6

Fixed.

I didn't knew about that. Thanks for sharing it with me.

[dblaikie]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1083

What happens if there are two subprograms using the same type? Do they both end up getting their CC changed? Maybe this code needs to get the type, duplicate it, then set it?

[RamNalamothu]

Address feedback.

[RamNalamothu]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1083

Ah, yes.

Fixed that. Thank you.

[RamNalamothu]

Removed llvm/IR/Metadata.h header inclusion.

[dblaikie]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1085–1088

Writing a loop to do this seems unfortunate when it should always be a known operand number - perhaps there's some nicer way to do this? (while keeping it maintainable/not hardcoding an operand number in this code)

Are there other examples of field updates we could draw inspiration from?

[RamNalamothu]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1085–1088

I couldn't figure out any other simpler way to do this.

I don't think the operand numbers are fixed/known (e.g. see llvm/test/DebugInfo/Generic/array.ll and llvm/test/DebugInfo/Generic/enum.ll).

And in all the code browsing I did, I haven't seen any other examples that could help with this. Please correct me if I missed something.

[RamNalamothu]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1085–1088

I don't think the operand numbers are fixed/known (e.g. see llvm/test/DebugInfo/Generic/array.ll and llvm/test/DebugInfo/Generic/enum.ll).

I mean, looking at DISubprogram node in those LIT tests convey that the type operand can be anywhere and not at a fixed position.

[scott.linder]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1085–1088

Can you define a new method DISubprogram::replaceType(DIType *T) { replaceOperandWith(4, T); } and use that? There is already e.g. void DISubprogram::replaceUnit(DICompileUnit *CU) { replaceOperandWith(5, CU); }

With either implementation, are we only ever mutating non-uniqued DISubprogram nodes? I believe DISubprogram nodes for declarations may be uniqued, but it seems like this pass can only be making changes when the definition is available, so we are safe?

[RamNalamothu]

Address feedback.

[RamNalamothu]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1085–1088

Thanks @scott.linder for the suggestion. Not sure how I missed this.

Yes, the DAE works only on the definitions and I have added an assert(isDistinct()) to the new method as well.

[dblaikie]

Sounds good, thanks!

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
   DEFINE_MDNODE_GET(DISubroutineType,
                     (DIFlags Flags, uint8_t CC, DITypeRefArray TypeArray),
                     (Flags, CC, TypeArray))
   DEFINE_MDNODE_GET(DISubroutineType,
                     (DIFlags Flags, uint8_t CC, Metadata *TypeArray),
                     (Flags, CC, TypeArray))
 
   TempDISubroutineType clone() const { return cloneImpl(); }
+  // Returns a new temporary DISubroutineType with updated CC
+  TempDISubroutineType cloneWithCC(uint8_t CC) const {
+    auto NewTy = clone();
+    NewTy->CC = CC;
+    return NewTy;
+  }
 
   uint8_t getCC() const { return CC; }
 
   DITypeRefArray getTypeArray() const {
     return cast_or_null<MDTuple>(getRawTypeArray());
   }
 
   Metadata *getRawTypeArray() const { return getOperand(3); }
public:
   void replaceLinkageName(MDString *LN) { replaceOperandWith(3, LN); }
 
   DISubroutineType *getType() const {
     return cast_or_null<DISubroutineType>(getRawType());
   }
   DIType *getContainingType() const {
     return cast_or_null<DIType>(getRawContainingType());
   }
+  void replaceType(DISubroutineType *Ty) {
+    assert(isDistinct() && "Only distinct nodes can mutate");
+    replaceOperandWith(4, Ty);
+  }
 
   DICompileUnit *getUnit() const {
     return cast_or_null<DICompileUnit>(getRawUnit());
   }
   void replaceUnit(DICompileUnit *CU) { replaceOperandWith(5, CU); }
   DITemplateParameterArray getTemplateParams() const {
     return cast_or_null<MDTuple>(getRawTemplateParams());
   }

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

 
 #include "llvm/Transforms/IPO/DeadArgumentElimination.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/IR/Argument.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
for (BasicBlock &BB : *NF)
       }
 
   // Clone metadatas from the old function, including debug info descriptor.
   SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
   F->getAllMetadata(MDs);
   for (auto MD : MDs)
     NF->addMetadata(MD.first, *MD.second);
 
+  // If either the return value(s) or argument(s) are removed, then probably the
+  // function does not follow standard calling conventions anymore. Hence add
+  // DW_CC_nocall to DISubroutineType to inform debugger that it may not safe to
+  // call this function or try to interpret the return value.
+  if (NFTy != FTy && NF->getSubprogram()) {
+    DISubprogram *SP = NF->getSubprogram();
+    auto Temp = SP->getType()->cloneWithCC(llvm::dwarf::DW_CC_nocall);
+    SP->replaceType(MDNode::replaceWithPermanent(std::move(Temp)));
+  }
+
   // Now that the old function is dead, delete it.
   F->eraseFromParent();
 
   return true;
 }
 
 PreservedAnalyses DeadArgumentEliminationPass::run(Module &M,
                                                    ModuleAnalysisManager &) {
   bool Changed = false;
 

llvm/test/Transforms/DeadArgElim/2010-04-30-DbgInfo.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature
 ; RUN: opt -S -passes=deadargelim < %s | FileCheck %s
 
+; Apart from checking if debug metadata is correctly propagated, this also tests whether DW_CC_nocall
+; calling convention is added when either return values or arguments are removed.
+
 @.str = private constant [1 x i8] zeroinitializer, align 1 ; <[1 x i8]*> [#uses=1]
 
-define i8* @vfs_addname(i8* %name, i32 %len, i32 %hash, i32 %flags) nounwind ssp {
+define i8* @vfs_addname(i8* %name, i32 %len, i32 %hash, i32 %flags) nounwind ssp !dbg !1 {
 ;
 entry:
   call void @llvm.dbg.value(metadata i8* %name, metadata !0, metadata !DIExpression()), !dbg !DILocation(scope: !1)
   call void @llvm.dbg.value(metadata i32 %len, metadata !10, metadata !DIExpression()), !dbg !DILocation(scope: !1)
   call void @llvm.dbg.value(metadata i32 %hash, metadata !11, metadata !DIExpression()), !dbg !DILocation(scope: !1)
   call void @llvm.dbg.value(metadata i32 %flags, metadata !12, metadata !DIExpression()), !dbg !DILocation(scope: !1)
 ; CHECK:  call fastcc i8* @add_name_internal(i8* %name, i32 %hash) [[NUW:#[0-9]+]], !dbg !{{[0-9]+}}
   %0 = call fastcc i8* @add_name_internal(i8* %name, i32 %len, i32 %hash, i8 zeroext 0, i32 %flags) nounwind, !dbg !13 ; <i8*> [#uses=1]
   ret i8* %0, !dbg !13
 }
 
 declare void @llvm.dbg.declare(metadata, metadata, metadata) nounwind readnone
 
-define internal fastcc i8* @add_name_internal(i8* %name, i32 %len, i32 %hash, i8 zeroext %extra, i32 %flags) noinline nounwind ssp {
+define internal fastcc i8* @add_name_internal(i8* %name, i32 %len, i32 %hash, i8 zeroext %extra, i32 %flags) noinline nounwind ssp !dbg !16 {
 ;
 entry:
   call void @llvm.dbg.value(metadata i8* %name, metadata !15, metadata !DIExpression()), !dbg !DILocation(scope: !16)
   call void @llvm.dbg.value(metadata i32 %len, metadata !20, metadata !DIExpression()), !dbg !DILocation(scope: !16)
   call void @llvm.dbg.value(metadata i32 %hash, metadata !21, metadata !DIExpression()), !dbg !DILocation(scope: !16)
   call void @llvm.dbg.value(metadata i8 %extra, metadata !22, metadata !DIExpression()), !dbg !DILocation(scope: !16)
   call void @llvm.dbg.value(metadata i32 %flags, metadata !23, metadata !DIExpression()), !dbg !DILocation(scope: !16)
   %0 = icmp eq i32 %hash, 0, !dbg !24             ; <i1> [#uses=1]

 !8 = !DIBasicType(tag: DW_TAG_base_type, name: "char", size: 8, align: 8, encoding: DW_ATE_signed_char)
 !9 = !DIBasicType(tag: DW_TAG_base_type, name: "unsigned int", size: 32, align: 32, encoding: DW_ATE_unsigned)
 !10 = !DILocalVariable(name: "len", line: 9, arg: 2, scope: !1, file: !2, type: !9)
 !11 = !DILocalVariable(name: "hash", line: 10, arg: 3, scope: !1, file: !2, type: !9)
 !12 = !DILocalVariable(name: "flags", line: 11, arg: 4, scope: !1, file: !2, type: !9)
 !13 = !DILocation(line: 13, scope: !14)
 !14 = distinct !DILexicalBlock(line: 12, column: 0, file: !28, scope: !1)
 !15 = !DILocalVariable(name: "name", line: 17, arg: 1, scope: !16, file: !2, type: !6)
+; CHECK: !DISubprogram(name: "add_name_internal"
+; CHECK-SAME: type: ![[MD:[0-9]+]]
 !16 = distinct !DISubprogram(name: "add_name_internal", linkageName: "add_name_internal", line: 22, isLocal: true, isDefinition: true, virtualIndex: 6, isOptimized: false, unit: !3, file: !28, scope: !2, type: !17)
+; CHECK: ![[MD]] = !DISubroutineType(cc: DW_CC_nocall, types: !{{[0-9]+}})
 !17 = !DISubroutineType(types: !18)
 !18 = !{!6, !6, !9, !9, !19, !9}
 !19 = !DIBasicType(tag: DW_TAG_base_type, name: "unsigned char", size: 8, align: 8, encoding: DW_ATE_unsigned_char)
 !20 = !DILocalVariable(name: "len", line: 18, arg: 2, scope: !16, file: !2, type: !9)
 !21 = !DILocalVariable(name: "hash", line: 19, arg: 3, scope: !16, file: !2, type: !9)
 !22 = !DILocalVariable(name: "extra", line: 20, arg: 4, scope: !16, file: !2, type: !19)
 !23 = !DILocalVariable(name: "flags", line: 21, arg: 5, scope: !16, file: !2, type: !9)
 !24 = !DILocation(line: 23, scope: !25)
 !25 = distinct !DILexicalBlock(line: 22, column: 0, file: !28, scope: !16)
 !26 = !DILocation(line: 24, scope: !25)
 !27 = !DILocation(line: 26, scope: !25)
 !28 = !DIFile(filename: "tail.c", directory: "/Users/echeng/LLVM/radars/r7927803/")
 !29 = !{}
 !30 = !{i32 1, !"Debug Info Version", i32 3}