[deadargelim] Update dbg.value of 'unused' parameters

[djtodoro]

Summary

DeadArgElim pass marks unused function arguments
as ‘undef’ without updating existing dbg.values referring to it.
As a consequence the debug info metadata in the final executable is wrong.

[djtodoro]

Please see the following lldb session:

...
(lldb) b f2
Breakpoint 1: where = test`f2 at test.c:4, address = 0x0000000000400460
(lldb) r
Process 12105 launched: './test' (x86_64)
Process 12105 stopped
* thread #1, name = 'test', stop reason = breakpoint 1.1
    frame #0: 0x0000000000400460 test`f2(k=1) at test.c:4
(lldb) p k
(int) $0 = 1
(lldb) register read
General Purpose Registers:
       rax = 0x0000000000400480  test`main at test.c:12
       rbx = 0x0000000000000000
       rcx = 0x0000000000000000
       rdx = 0x00007fffffffdcd8
       rdi = 0x0000000000000001
       rsi = 0x00007fffffffdcc8
       rbp = 0x00000000004004a0  test`__libc_csu_init
       rsp = 0x00007fffffffdbd8
        r8 = 0x0000000000400490  test`__libc_csu_fini
        r9 = 0x00007ffff7de7ab0  ld-linux-x86-64.so.2`___lldb_unnamed_symbol57$$ld-linux-x86-64.so.2
       r10 = 0x0000000000000846
       r11 = 0x00007ffff7a2d740  libc.so.6`__libc_start_main at libc-start.c:134
       r12 = 0x0000000000400370  test`_start
       r13 = 0x00007fffffffdcc0
       r14 = 0x0000000000000000
       r15 = 0x0000000000000000
       rip = 0x0000000000400460  test`f2 at test.c:4
    rflags = 0x0000000000000246
        cs = 0x0000000000000033
        fs = 0x0000000000000000
        gs = 0x0000000000000000
        ss = 0x000000000000002b
        ds = 0x0000000000000000
        es = 0x0000000000000000
...

So, parameter 'k' does not have value 1, it is wrong. After applying the patch the value will be optimized out.
C code of the test case can be found as a comment in the attached llvm test.

[aprantl]

Thanks, this looks mostly good, minor comments for the testcase inline.

Why doesn't this do the right thing automatically though? What does the dbg.value point to after %k is dropped?

test/Transforms/DeadArgElim/dbginfo-update-dbgval.ll

27

can you also add a check that f2 no longer has an argument?

79

You can probably strip out all TBAA metadata, it just distracts in this testcase.

[vsk]

In D51968#1232061, @aprantl wrote:

Thanks, this looks mostly good, minor comments for the testcase inline.

Why doesn't this do the right thing automatically though? What does the dbg.value point to after %k is dropped?

Because %k isn't deleted (I have an explanation in an inline comment). This part of DAE just edits calls to 'f2' s.t they pass in undef wherever they would have passed in a "real" value for %k.

lib/Transforms/IPO/DeadArgumentElimination.cpp

295

I think you need Changed = true here. Otherwise, this will return false if Fn has no uses.

test/Transforms/DeadArgElim/dbginfo-update-dbgval.ll

27

DAE doesn't remove the argument here, see the comment from SurveyFunction:

// We consider arguments of non-internal functions to be intrinsically alive as
// well as arguments to functions which have their "address taken".

[aprantl]

LGTM with TBAA cruft removed.

test/Transforms/DeadArgElim/dbginfo-update-dbgval.ll

27

I see. That's fine then.

[djtodoro]

@aprantl Thanks for the review! @vsk already answered all questions.

[djtodoro]

@vsk Thanks for the review!

It sounds reasonable to set 'Changed = true' even we change debug info metadata, so I have addressed it with the diff updated.

[vsk]

Thanks, LGTM as well.

lib/Transforms/IPO/DeadArgumentElimination.cpp

294

Nit, add a space before the '('?

lib/Transforms/IPO/DeadArgumentElimination.cpp

bool DeadArgumentEliminationPass::RemoveDeadArgumentsFromCallers(Function &Fn) {
   if (Fn.hasFnAttribute(Attribute::Naked))
     return false;
 
   if (Fn.use_empty())
     return false;
 
   SmallVector<unsigned, 8> UnusedArgs;
   for (Argument &Arg : Fn.args()) {
-    if (!Arg.hasSwiftErrorAttr() && Arg.use_empty() && !Arg.hasByValOrInAllocaAttr())
+    if (!Arg.hasSwiftErrorAttr() && Arg.use_empty() && !Arg.hasByValOrInAllocaAttr()) {
+      if(Arg.isUsedByMetadata())
+        Arg.replaceAllUsesWith(UndefValue::get(Arg.getType()));
       UnusedArgs.push_back(Arg.getArgNo());
     }
+  }
 
   if (UnusedArgs.empty())
     return false;
 
   bool Changed = false;
 
   for (Use &U : Fn.uses()) {
     CallSite CS(U.getUser());

test/Transforms/DeadArgElim/dbginfo-update-dbgval.ll

+; RUN: opt -deadargelim -S < %s | FileCheck %s
+;test.c
+;int s;
+;
+;void f2(int k) __attribute__((noinline)) {
+; s++;
+; k = s;
+;}
+;
+;void f() __attribute__((noinline)) {
+; f2(4);
+;}
+;
+;int main()
+;{
+; f();
+; return 0;
+;}
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+@s = common dso_local local_unnamed_addr global i32 0, align 4, !dbg !0
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local void @f2(i32 %k) local_unnamed_addr #0 !dbg !11 {
+entry:
+; CHECK: call void @llvm.dbg.value(metadata i32 undef, metadata !15, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata i32 %k, metadata !15, metadata !DIExpression()), !dbg !16
+  %0 = load i32, i32* @s, align 4, !dbg !17, !tbaa !18
+  %inc = add nsw i32 %0, 1, !dbg !17
+  store i32 %inc, i32* @s, align 4, !dbg !17, !tbaa !18
+  call void @llvm.dbg.value(metadata i32* @s, metadata !15, metadata !DIExpression(DW_OP_deref)), !dbg !16
+  ret void, !dbg !22
+}
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local void @f() local_unnamed_addr #0 !dbg !23 {
+entry:
+; CHECK: tail call void @f2(i32 undef), !dbg !26
+  tail call void @f2(i32 4), !dbg !26
+  ret void, !dbg !27
+}
+
+; Function Attrs: nounwind uwtable
+define dso_local i32 @main() local_unnamed_addr #1 !dbg !28 {
+entry:
+  tail call void @f(), !dbg !31
+  ret i32 0, !dbg !32
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!7, !8, !9}
+!llvm.ident = !{!10}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "s", scope: !2, file: !3, line: 1, type: !6, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 8.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5)
+!3 = !DIFile(filename: "test.c", directory: "/dir")
+!4 = !{}
+!5 = !{!0}
+!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!7 = !{i32 2, !"Dwarf Version", i32 4}
+!8 = !{i32 2, !"Debug Info Version", i32 3}
+!9 = !{i32 1, !"wchar_size", i32 4}
+!10 = !{!"clang version 7.0.0"}
+!11 = distinct !DISubprogram(name: "f2", scope: !3, file: !3, line: 3, type: !12, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !2, retainedNodes: !14)
+!12 = !DISubroutineType(types: !13)
+!13 = !{null, !6}
+!14 = !{!15}
+!15 = !DILocalVariable(name: "k", arg: 1, scope: !11, file: !3, line: 3, type: !6)
+!16 = !DILocation(line: 3, column: 13, scope: !11)
+!17 = !DILocation(line: 4, column: 3, scope: !11)
+!18 = !{!19, !19, i64 0}
+!19 = !{!"int", !20, i64 0}
+!20 = !{!"omnipotent char", !21, i64 0}
+!21 = !{!"Simple C/C++ TBAA"}
+!22 = !DILocation(line: 6, column: 1, scope: !11)
+!23 = distinct !DISubprogram(name: "f", scope: !3, file: !3, line: 8, type: !24, isLocal: false, isDefinition: true, scopeLine: 8, isOptimized: true, unit: !2, retainedNodes: !4)
+!24 = !DISubroutineType(types: !25)
+!25 = !{null}
+!26 = !DILocation(line: 9, column: 2, scope: !23)
+!27 = !DILocation(line: 10, column: 1, scope: !23)
+!28 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 12, type: !29, isLocal: false, isDefinition: true, scopeLine: 12, isOptimized: true, unit: !2, retainedNodes: !4)
+!29 = !DISubroutineType(types: !30)
+!30 = !{!6}
+!31 = !DILocation(line: 13, column: 2, scope: !28)
+!32 = !DILocation(line: 14, column: 2, scope: !28)