[PATCH] D61890: [LiveDebugValues] End variable's range with multiple locations at block entry

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Hmmm, I get what the patch is doing, but I have the feeling that it might be the wrong place (or impossible) to fix in LiveDebugValues. Given the sample source in dbg-live-debug-values-end-range.mir, would I be right in thinking that this patch is to fix the ChangingRegs behaviour that David brought up in [0]?

Consider this example:

  volatile int g, *x;
  
  int baz(int p, int q) {
    int local;
  
    switch (g) {
    case 1:
      local = p;
      x[1] = local;
      break; 
    case 2:
      x[2] += p;
      break;
    default:
      local = q;
      g++;
      break;
    }
  
    return x[1] / q + g;
  }

Compiling with llvm r359863 and this patch applied, the conflicting locations of "local" in the "case 1" and default blocks don't seem to be detected, and no DBG_VALUE $noreg is produced in the function exit block, which is what I anticipated this patch would do. I haven't dug into why. As a result, the "default" location of "local" flows into the exit block due to the ChangingRegs functionality (see below).

IMHO there's a bigger problem though, assuming this patch is about fixing the behaviour in [0]. LiveDebugValues only deals with the control flow predecessors / successors of blocks when calculating LiveIns / LiveOuts, wheras DbgEntityHistoryCalculator considers blocks only in the order that they're placed in. Thus, terminating a variable location in the control flow successor block won't stop DbgEntityHistoryCalculator extending a location into the next block in the placement order. If the next placed block isn't a control flow successor to the previously placed block, it won't get a DBG_VALUE $noreg. That can been seen in the example above: according to llvm-dwarfdump, the "case 2" block (0x1c -> 0x28) gets a location for the "local" variable despite it having no location in the source / pre-backend-IR:

  $ clang-w-patch test2.c -o out.o -O2 -g -c
  $ objdump -d out.o
    0000000000000000 <baz>:
       0:	8b 05 00 00 00 00    	mov    0x0(%rip),%eax        # 6 <baz+0x6>
       6:	83 f8 02             	cmp    $0x2,%eax
       9:	74 11                	je     1c <baz+0x1c>
       b:	83 f8 01             	cmp    $0x1,%eax
       e:	75 18                	jne    28 <baz+0x28>
      10:	48 8b 05 00 00 00 00 	mov    0x0(%rip),%rax        # 17 <baz+0x17>
      17:	89 78 04             	mov    %edi,0x4(%rax)
      1a:	eb 1a                	jmp    36 <baz+0x36>
      1c:	48 8b 05 00 00 00 00 	mov    0x0(%rip),%rax        # 23 <baz+0x23>
      23:	01 78 08             	add    %edi,0x8(%rax)
      26:	eb 0e                	jmp    36 <baz+0x36>
      28:	83 05 00 00 00 00 01 	addl   $0x1,0x0(%rip)        # 2f <baz+0x2f>
      2f:	48 8b 05 00 00 00 00 	mov    0x0(%rip),%rax        # 36 <baz+0x36>
      36:	8b 40 04             	mov    0x4(%rax),%eax
      39:	99                   	cltd   
      3a:	f7 fe                	idiv   %esi
      3c:	03 05 00 00 00 00    	add    0x0(%rip),%eax        # 42 <baz+0x42>
      42:	c3                   	retq   
  
  $ llvm-dwarfdump-7 --name=local out.o
  
    0x00000098: DW_TAG_variable
              DW_AT_location	(0x00000000
                 [0x0000000000000010,  0x0000000000000028): DW_OP_reg5 RDI
                 [0x0000000000000028,  0x0000000000000043): DW_OP_reg4 RSI)
              DW_AT_name	("local")

I suspect the only way of fixing this in general is killing off the ChangingRegs functionality in the DbgEntityHistoryCalculator. I've given that an initial look and it raises some awkward questions; I'll upload the patch for discussion in a moment.

For the avoidance of doubt: I don't think fixing ChangingRegs is required for D61600 <https://reviews.llvm.org/D61600> (it's just something that came up in discussion), I'll post a comment there to make that clear.

[0] https://reviews.llvm.org/D61600#1493393


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

https://reviews.llvm.org/D61890