[PATCH] D85555: [InstCombine] Remove dbg.values describing contents of dead allocas

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added subscribers: chrisjackson, jmorse.
jmorse added a comment.

NB: on the topic of LowerDbgDeclare, it was discussed extensively in PR34136 (CC @rnk) , and there's actually a plan for eliminating its behaviour there [0] from about two years ago, which we were eventually going to look at (CC @chrisjackson).

Everything to do with leaked pointers seems to have a bunch of compromises -- I think this patch alters some of them, and I don't have a strong opinion as to whether it's good or not. With `dbg.value`s placed at call sites:

1. The variable might refer to a stack location containing a stale value, if a store was DSE'd,
2. The stack location might be optimised out (which is what this patch addresses),
3. But if the callee stores to the stack location, the variable location accurately reflects this.

By fixing 2, we lose some of 3: In the test case you add, if you add a:

  store i32 1, i32* %addr, align 4

To the 'use' function, then t2 doesn't reflect this after -instcombine -inline -instcombine:

  define void @t2(i32 %x) !dbg !17 {
    call void @llvm.dbg.value(metadata i32 %x, metadata !18, metadata !DIExpression()), !dbg !19
    ret void
  }

Specifically, the "old" value %x would be presented as the variable value on the 'ret 'instruction, wheras at -O0 the variable location on the stack would contain the '1' stored by 'use'. Without this patch, the variable is reported as optimized-out.

I don't really see this as being a bug:  we just don't cope well with leaked pointers, and stale values can appear as a result.

[0] https://bugs.llvm.org/show_bug.cgi?id=34136#c25


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85555/new/

https://reviews.llvm.org/D85555