[PATCH] D80691: Proposed fix for PR46114

Fri May 29 16:57:29 PDT 2020

helloqirun marked an inline comment as done.
helloqirun added a comment.

In D80691#2062323 <https://reviews.llvm.org/D80691#2062323>, @Orlando wrote:

> 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.

Indeed it shows a 0, as I mentioned in the summary. See the following.

  $ clang -v
  clang version 10.0.0 (https://github.com/llvm/llvm-project.git 632deb6bd04022945468faef2dcaa8c9fdf1b0fd)
  $ clang -g -O3 abc.c
  $ cat abc.ll
  ; ModuleID = 'abc.c'
  source_filename = "abc.c"
  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
  target triple = "x86_64-unknown-linux-gnu"

  @a = dso_local local_unnamed_addr global i32 1, align 4, !dbg !0
  @c = common dso_local local_unnamed_addr global i8 0, align 1, !dbg !9
  @b = common dso_local local_unnamed_addr global i16 0, align 2, !dbg !6
  ; Function Attrs: nofree norecurse nounwind uwtable
  define dso_local i32 @main() local_unnamed_addr #0 !dbg !17 {
  entry:
    %0 = load i32, i32* @a, align 4, !dbg !26, !tbaa !27
    call void @llvm.dbg.value(metadata i32 %0, metadata !21, metadata !DIExpression()), !dbg !31
    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
    %1 = load i8, i8* @c, align 1, !dbg !32, !tbaa !33
    %conv = sext i8 %1 to i32, !dbg !32

  $ lldb-trunk -s cmds -b a.out
  (lldb) target create "a.out"
  (lldb) b 6
  Breakpoint 1: where = a.out`main + 6 at abc.c:6:20, address = 0x0000000000400486
  (lldb) r
  Process 13524 stopped
  * thread #1, name = 'a.out', stop reason = breakpoint 1.1
      frame #0: 0x0000000000400486 a.out`main at abc.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   	  }

  Process 13524 launched: '/home/absozero/bisect/fix/a.out' (x86_64)
  (lldb) p l_52
  (int) $0 = 0

  $ llvm-dwarfdump a.out -name l_52
  a.out:	file format ELF64-x86-64

  0x00000097: DW_TAG_variable
                DW_AT_location	(0x00000000:
                   [0x0000000000400486, 0x0000000000400499): DW_OP_consts +0, DW_OP_stack_value)
                DW_AT_name	("l_52")
                DW_AT_decl_file	("/home/absozero/bisect/fix/abc.c")
                DW_AT_decl_line	(5)
                DW_AT_type	(0x0000003f "int")

That's precisely the motivation behind my patch. I don't know an immediate way in earlyCSE to remove the second `dbg.value`. The backward scan in `RemoveRedundantDbgInstrs` will always eliminate the first `dbg.value`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80691