[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

1090–1091

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

1090–1091

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

1093

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

1093

Ah, yes.

Fixed that. Thank you.

[RamNalamothu]

Removed llvm/IR/Metadata.h header inclusion.

[dblaikie]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

1095–1098

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

1095–1098

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

1095–1098

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

1095–1098

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

1095–1098

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

DEFINE_MDNODE_GET(DISubroutineType,
                     (Flags, CC, TypeArray))
   DEFINE_MDNODE_GET(DISubroutineType,
                     (DIFlags Flags, uint8_t CC, Metadata *TypeArray),
                     (Flags, CC, TypeArray))
 
   TempDISubroutineType clone() const { return cloneImpl(); }
 
   uint8_t getCC() const { return CC; }
+  void setCC(uint8_t NewCC) { CC = NewCC; }
 
   DITypeRefArray getTypeArray() const {
     return cast_or_null<MDTuple>(getRawTypeArray());
   }
 
   Metadata *getRawTypeArray() const { return getOperand(3); }
 
   static bool classof(const Metadata *MD) {

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 all the return values are removed, add DW_CC_nocall to DISubroutineType
+  // so that debuggers will know it is not safe to call this function or try to
+  // interpret the return value.
+  // FIXME: Should we add DW_CC_nocall simply when NFTy != FTy i.e. whenever
+  // an unused return value or an argument is removed?
+  if (!RetTy->isVoidTy() && NRetTy->isVoidTy() && NF->getSubprogram())
+    // Change CallingConvention to DW_CC_nocall
+    NF->getSubprogram()->getType()->setCC(llvm::dwarf::DW_CC_nocall);
+
   // Now that the old function is dead, delete it.
   F->eraseFromParent();
 
   return true;
 }
 
 PreservedAnalyses DeadArgumentEliminationPass::run(Module &M,
                                                    ModuleAnalysisManager &) {
   bool Changed = false;
 
   // First pass: Do a simple check to see if any functions can have their "..."

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
+; RUN: opt < %s -S -passes=deadargelim > %t
+; RUN: cat %t | FileCheck %s
+; Test whether DW_CC_nocall calling convention is added when return values are removed and return type changed to void.
+; RUN: cat %t | grep "define internal fastcc void \@add_name_internal"
+; RUN: cat %t | grep DW_CC_nocall
 
 @.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]+}}
+; CHECK:  call fastcc void @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
+  ret i8* %name, !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]