Page MenuHomePhabricator

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

Authored by djtodoro on Sep 12 2018, 1:42 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

djtodoro created this revision.Sep 12 2018, 1:42 AM

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.

djtodoro edited the summary of this revision. (Show Details)Sep 12 2018, 1:56 AM

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 ↗(On Diff #165030)

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

79 ↗(On Diff #165030)

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

vsk added a comment.Sep 12 2018, 11:26 AM

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 ↗(On Diff #165030)

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 ↗(On Diff #165030)

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 accepted this revision.Sep 12 2018, 12:31 PM

LGTM with TBAA cruft removed.

test/Transforms/DeadArgElim/dbginfo-update-dbgval.ll
27 ↗(On Diff #165030)

I see. That's fine then.

This revision is now accepted and ready to land.Sep 12 2018, 12:31 PM
djtodoro updated this revision to Diff 165249.Sep 13 2018, 4:32 AM
djtodoro marked 2 inline comments as done.Sep 13 2018, 4:36 AM

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

@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 accepted this revision.Sep 13 2018, 11:45 AM

Thanks, LGTM as well.

lib/Transforms/IPO/DeadArgumentElimination.cpp
296 ↗(On Diff #165249)

Nit, add a space before the '('?

djtodoro updated this revision to Diff 165439.Sep 14 2018, 1:10 AM
djtodoro marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.