[PATCH] D59431: [DebugInfo] Terminate constant-value location-list ranges at the end of basic blocks

Tue Mar 19 13:46:49 PDT 2019

jmorse added a comment.

In D59431#1431410 <https://reviews.llvm.org/D59431#1431410>, @aprantl wrote:

> Isn't that a symptom of previous passes failing to insert a dbg.value(undef) when they deleted a dbg.value? I'm trying to figure out what a legitimate example for this would look like.

Awkwardly yes and no -- the pass that doesn't insert dbg.value(undef) is mem2reg constructing SSA IR (example below). However LiveDebugValues and DbgEntityHistoryCalculator normally cover for this issue and appear to be designed to do so. I'm having a hard time describing what's wrong here, so I'm going to state two beliefs; explain how they works with register variable locations right now; and then how it doesn't work with constant variable locations (hence this patch). It's my (additional) belief that fixing these things at the LLVM-IR level would require an infeasibly large number of dbg.value intrinsics to be inserted, in the order of "maximal SSA".

The two beliefs are:

- We eventually get (and need) a DBG_VALUE for every source variable in every basic block recording its location for that block, and
- When control flow merges, we infer variable locations from the parent blocks, or terminate locations if they're inconsistent.

The first seems obvious: to make linear ranges of locations for DWARF location lists, we need to scan the machine code and record variable locations. However AFAIUI the IR has no control over how the target backend will chose to lay out blocks. Consider this example where I've reversed the control flow:

  define dso_local i32 @_Z3foob(i1 %cond) #0 !dbg !12 {
  entry:
    br label %bb2

  exit: 
    ret i32 %value, !dbg !30

  bb1: 
    br label %exit, !dbg !26

  bb2:
    %value = select i1 %cond, i32 0, i32 1 
    call void @llvm.dbg.value(metadata i32 %value, metadata !23, metadata !DIExpression()), !dbg !24
    br label %bb1
  }

When compiled with llc -O0 the block layout is kept the same in assembly (although that's not guaranteed). The dbg.value dominates the 'bb1' and 'exit' blocks, and when using a register in the example here it's location is correctly propagated into those blocks. However if one replaces the dbg.value operand with a constant, the location list only covers the last jmp of the machine code in 'bb2'. Furthermore, if you put a dbg.value of a different constant in the 'entry' block, according to the IR it dominates only "br label %bb2" and the select, but in the location-lists it will cover the whole function up to the last jmp. The main message here is that IR dominance is manually preserved by LiveDebugValues/DbgEntityHistoryCalculator for registers, but not constants, and because block layout is under the targets control the IR cannot fix this without wholesale copying of dbg.values into all blocks.

Problem 2, what to do when control flow merges, can be demonstrated with this example C++:

  extern int f();
  int global = 0;
  int foo(bool cond) {
      int bar;
      if (cond) {
          bar = f();
          global = bar;
      } else {
          bar = f();
          global = bar;
      }
      return bar;
  }

The variable of interest is going to be "bar". If I compile it with the following chain of things, which I believe disables all optimisations except mem2reg, using a month-old r354365:

  clang++ ./test.cpp -g -Xclang -disable-O0-optnone -Xclang -disable-llvm-optzns -emit-llvm -c -S -o - | opt -mem2reg -S | llc -o - -O0 -filetype=obj | llvm-dwarfdump-6.0 -

Then some fairly obvious things happen: we get a dbg.value for "bar" on each side of the conditional branch, a PHI node in the exit block to merge the two values of "bar", a dbg.value for the PHI, then a return instruction. This is unremarkable: but interesting things start happening if "return bar" is replaced by "return 0". In that case, the PHI node in the exit block disappears, as does the dbg.value in the exit block. We're left with this IR after mem2reg:

  ; <label>:4:
    %5 = call i32 @_Z1fv(), !dbg !21
    call void @llvm.dbg.value(metadata i32 %5, metadata !23, metadata !DIExpression()), !dbg !24
    store i32 %5, i32* @global, align 4, !dbg !25
    br label %9, !dbg !26

  ; <label>:6:
    %7 = call i32 @_Z1fv(), !dbg !27
    call void @llvm.dbg.value(metadata i32 %7, metadata !23, metadata !DIExpression()), !dbg !24
    store i32 %7, i32* @global, align 4, !dbg !29
    br label %8

  ; <label>:8:
    ret i32 0, !dbg !31

Here there's no explicit termination of the live range of values of "bar" in %5 and %7, either in IR or in debug intrinsics. "bar" isn't used after control flow merges into the exit/return block, hence no phi or dbg.value. However, there is also no dbg.value(undef) terminating the variable. Instead, for registers, LiveDebugValues determines where variable locations merge and either adds a DBG_VALUE for the merged location, or nothing if it can't determine a location, which implicitly terminates the location range. Neither the propagation nor the termination happen for constants (fixed by this patch).

If we were to fix mem2reg to insert dbg.value(undef) in these circumstances I believe we'd have to introduce a large number of these intrinsics on every control flow merge, and we'd effectively be embedding the early liveness properties of the program into the debug metadata, even if the liveness then changes over the course of optimisation. Plus, the design clearly works for registers right now, it's just not implemented for constants.

In summary, I think LiveDebugValues/DbgEntityHistoryCalculator are inferring facts about variable locations to avoid having to record all variable locations in all basic blocks at the start of compilation. However, their implementation is not complete, as they mostly ignore constant locations. IMO it's a better fix to make their implementation complete, rather than to increase the number of dbg.values that have to be recorded.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59431