[PATCH] D80691: Proposed fix for PR46114

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando requested changes to this revision.
Orlando added a comment.
This revision now requires changes to proceed.

I think that this patch reduces variable coverage by undefing dbg.values which
it cannot salvage that would otherwise be RAUWd.

I'd expect your call to salvageDebugInfoOrMarkUndef to insert an 'undef'
dbg.value for 'l_52' for your original reproducer because you cannot salvage a
load operation. When I build with this patch locally, that is what I get. lldb
(and gdb) tells me that the value for 'l_52' has been optimized out.

With your patch applied, using your original reproducer:

  $ cat test.c
  int a = 1;
  short b;
  char c;
  int main() {
    int d = a, l_52 = 0;
    b = (l_52 = a) - c; //   qirun0
    if (d) {
      int e=1;
    }
  }
  
  $ clang-with-your-patch -O3 -g test.c
  $ lldb a.out
  ...
  (lldb) s
  Process 21970 stopped
  * thread #1, name = 'a.out', stop reason = step in
      frame #0: 0x0000000000400486 a.out`main at test.c:6:20
     3   	char c;
     4   	int main() {
     5   	  int d = a, l_52 = 0;
  -> 6   	  b = (l_52 = a) - c; //   qirun0
     7   	  if (d) {
     8   	    int e=1;
     9   	  }
  (lldb) p l_52
  error: Couldn't materialize: couldn't get the value of variable l_52: no location, value may have been optimized out

My lldb is built at 9b56cc9361a471a3ce57da4c98f60e377cc43026 <https://reviews.llvm.org/rG9b56cc9361a471a3ce57da4c98f60e377cc43026> (1st April 2020),
and clang 709c52b9553 <https://reviews.llvm.org/rG709c52b9553f4ba83ab901a873392f8a7f2c56a5> (18th May 2020). What do you see in lldb when you build
your original reproducer with this patch?

> There are cases where EarlyCSE needs to remove the first (the following case). The instruction `%0 = load i32, i32* @a, align 4` can be the product of both EarlyCSE DCE (the following case) and EarlyCSE CSE LOAD (the case in the original PR).

My understanding is that DCE and CSE do not have equal implications for
debug-info. DCE means the value is going away because it isn't used.  In this
case we should salvage any debug users (i.e. dbg.values using the value),
because the value will no longer exist.  For CSE the value still exists
somewhere, so we instead want to update debug users to use the value which we
are keeping. This is what the line `Inst.replaceAllUsesWith(Op);` is doing
without your patch. However, with your patch applied, the salvage call replaces
the dbg.values with a constant if it can salvage, or undef if it cannot. This
means that the replaceAllUsesWith (RAUW) cannot update the debug users.

> Thanks! Yes, if we want to have two dbg.value in the IR, it works fine. That's precisely what was before e5f07080b8a <https://reviews.llvm.org/rGe5f07080b8ac73dd573e81a186925a122ab8a39d>, i.e., the final IR will contain
> 
> call void @llvm.dbg.value(metadata i32 0, metadata !22, metadata !DIExpression()), !dbg !31
>  call void @llvm.dbg.value(metadata i32 %0, metadata !22, metadata !DIExpression()), !dbg !31
>  and lldb can print the correct value of 0.

I wouldn't expect this to result in you seeing a value of '0' in the debugger.

>From SourceLevelDebugging.rst [1]:

1. A dbg.value terminates the effect of any preceding dbg.values for (any overlapping fragments of) the specified variable.
2. The dbg.value’s position in the IR defines where in the instruction stream the variable’s value changes.

Debug intrinsics (e.g. llvm.dbg.value) will not be lowered into "real"
instructions.  This means that you cannot step onto them. Instead, they mark
out ranges of instructions where a variable location is live. So when you have
a contiguous pack of them like this referring to the same variable only the
last one has any effect.

For sanity, please could you run the following command on the reproducer built with
clang with and without your patch?

$ llvm-dwarfdump a.out -name l_52

Thanks,
Orlando

[1] https://llvm.org/docs/SourceLevelDebugging.html#object-lifetime-in-optimized-code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80691