[PATCH] D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass.

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This is a great bug find -- I'd no idea SROA ran twice! Applying this patch gives a ~3% increase in variable locations covered on a clang-3.4 build.

However, IMHO converting dbg.values to dbg.declares is going to prove problematic. There's no (AFAIK) good way to confirm that a dbg.value was originally _supposed_ to be a dbg.declare, and so dbg.values that accidentally refer to an alloca might be converted. There are all kinds of optimisations and salvaging that could make dbg.values point back at an alloca.

For example, with your current patch applied to r365447, if I add a pointer "ptr" that points at the alloca, and another field to the struct:

  struct S1 {
      int p1;
      long int p2; // jmorse
      
      bool IsNull (  ) {
          return p1 == 0;
      }
  };
      
  S1 foo ( void );
      
  int bar (  ) {
      
      S1 result = foo();
      S1 * ptr = &result; //jmorse
      
      if ( result.IsNull() )
          return 0;
      
      return result.p1 + 1;
  }

And run "./bin/clang++ -O2 -g test.c -o test.o -c", then objdump and llvm-dwarfdump, the alloca gets promoted:

  0000000000000000 <_Z3barv>:
     0:   50                      push   %rax
     1:   e8 00 00 00 00          callq  6 <_Z3barv+0x6>
                          2: R_X86_64_PLT32       _Z3foov-0x4
     6:   8d 48 01                lea    0x1(%rax),%ecx
     9:   85 c0                   test   %eax,%eax
     b:   0f 45 c1                cmovne %ecx,%eax
     e:   59                      pop    %rcx
     f:   c3                      retq

But ptr isn't optimised out as it should be. According to dwarfdump:

  0x000000b1: DW_TAG_variable
                DW_AT_location    (0x00000000
                   [0x0000000000000006,  0x000000000000000e): DW_OP_reg0 RAX, DW_OP_piece 0x4)
                DW_AT_name        ("ptr")
                DW_AT_decl_file   ("/faster/fs/release/test.c")
                DW_AT_decl_line   (15)
                DW_AT_type        (0x0000008f "S1*")

The location is given as $eax, which actually contains one of the struct fields, and would be misleading. This is down to its dbg.value becoming a dbg.declare, when really it should have been dropped. (This doesn't happen if you don't add the extra "p2" field due to ConvertDebugDeclareToDebugValue being confused at types that don't make sense).

One potential workaround might be to examine the DIExpression to see whether or not it actually dereferences the alloca address and has an aggregate type. However that means digging through the DIExpression operands, which I believe can become complicated if a lot of optimization has happened. There might also be non-C/C++ frontends that expect to be able to temporarily bind variable names to memory, then move them later. Converting their dbg.values to dbg.declares would change the duration that the variable referred to a particular piece of memory.

~

Instead, maybe we could just not convert dbg.declare for structs to dbg.values during instcombine in the first place? While digging into this I noticed that the comment at [0] says only scalars are supposed to be dbg.declare-converted during instcombine, but the code allows structs to slip through. I don't know the history of LowerDbgDeclare though, do other debug-info people know why structs get converted there?

~

For the avoidance of doubt, this is a great find with a big coverage win, but I think we need a safer way of finding the defective dbg.values (or; not converting them in the first place).

[0] https://github.com/llvm/llvm-project/blob/db101864bdc938deb1d63fe4f7da761bd38e5cae/llvm/lib/Transforms/Utils/Local.cpp#L1419


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

https://reviews.llvm.org/D64595