[PATCH] D70121: [DebugInfo][LDV] Teach LDV how to identify source variables and handle fragments

Thu Jan 30 08:23:18 PST 2020

Orlando added a comment.

@vsk this is the situation as far as I understand it. I appreciate that this is
a lot of text; there is a summary at the end. I hope this helps.

> Apologies, I'm not up-to-date on this patch and would appreciate some
>  clarification / a few short examples of what overlapping fragments are.  Is it
>  when you have adjacent dbg.value / DBG_VALUEs, s.t. there is some overlap in
>  the bits they describe?

Yes, except adjacency is not part of that relationship.

  Example 5
  DBG_VALUE %0, $noreg, "x", DIExpression() ; DV1
  ...
  DBG_VALUE %0, $noreg, "y", DIExpression(DW_OP_LLVM_fragment 0 16) ; DV2
  ...
  DBG_VALUE %1, $noreg, "x", DIExpression() ; DV3 
  ...
  DBG_VALUE %0, $noreg, "y", DIExpression(DW_OP_LLVM_fragment 8 8)  ; DV4

DV1, DV3, have fully overlapping fragments.  DV2 and DV4 have partially
overlapping fragments.

So, in your example 4 the first and last DBG_VALUEs partially overlap.

> are these valid examples?

  Example 1: Correct - I will explicitly state "full overlap" if I must refer
             to this case.  
  Example 2: Correct.
  Example 3: Correct.
  Example 4: Not quite, that's still a partial overlap. See above.

> I'm fuzzy on the definition here so apologies for the probably naive question,
>  but: why are overlaps a problem?

Having to consider fragment overlaps complicates value liveness queries for any
particular bits of a source variable. 'value' here refers to the combination of
a location (e.g. vreg, const) and DIExpression (because this may add a modifier
to the location).

> Would you mind following up on the review for D71478 <https://reviews.llvm.org/D71478> describing what it's
>  missing?

Ah, looking closely at D71478 <https://reviews.llvm.org/D71478> the pass will never produce wrong debuginfo. It
doesn't remove any dbg.values which _may overlap_ by ignoring the fragment part.
It takes a conservative approach, but this has no effect on the final debuginfo
so it turns out it isn't a very compelling example.

In the Example 6 below it would be safe to remove DV3, but the pass
implemented in D71478 <https://reviews.llvm.org/D71478> will not currently do so because DV3 and DV2 _may
overlap_.

  Example 6
  dbg.value %1, "x", Fragment(0, 16) ; DV1
  ...
  dbg.value %2, "x", Fragment(16, 16) ; DV2
  ...
  dbg.value %1, "x", Fragment(0, 16) ; DV3

> why are overlaps a problem? (continued)

LiveDebugVariables (LDV) removes DBG_VALUEs before regalloc and reinserts after.
It cannot afford to be so conservative because this reduces coverage. Before I
explain this I have to show you what is wrong with LDV currently.

LDV uses interval maps to explicitly represent the intervals that the removed
DBG_VALUEs implicitly described. The DBG_VALUEs are filtered into an interval
map based on their { Source Variable, DIExpression }. The interval map will
coalesce adjacent entries that use the same location.

Consider (interval maps before coalescing on the left):

  Example 7
  +------------DBG_VALUE %0, $noreg, "x", DIExpression() ; DV1
  |            ...
  |       +----DBG_VALUE %1, $noreg, "x", DIExpression(DW_OP_plus_uconst, 4, DW_OP_stack_value) ; DV2
  |       |    ...    
  +-------|----DBG_VALUE %0, $noreg, "x", DIExpression() ; DV3
  |       |
  DV1(%0) DV2(%1)
  DV3(%0)

This approach generates incorrect debuginfo. The interval map coalesces DV3 into
DV1 because they use the same location.

Taking the same approach as D71478 <https://reviews.llvm.org/D71478> and filtering only on { Source Variable }
would fix the bug described above but also may greatly reduce coverage. For
example:

  Example 8
  +----DBG_VALUE %0, "x", (DW_OP_LLVM_fragment 0 8) ; DV1
  |    ...
  +----DBG_VALUE %1, "x", (DW_OP_LLVM_fragment 8 8) ; DV2
  |  
  +----DBG_VALUE %0, "x", (DW_OP_LLVM_fragment 0 8) ; DV3
  |
  DV1(%0)
  DV2(%1)
  DV3(%0)

In Example 8 DV3 is not coalesced into DV1. Imagine that regalloc wants to
spill %0:

  Example 9
  +----DBG_VALUE %0, "x", (DW_OP_LLVM_fragment 0 8) ; DV1
  |    ...
  +----DBG_VALUE %1, "x", (DW_OP_LLVM_fragment 8 8) ; DV2
  |    <SPILL %0 into STACK0>
  +----DBG_VALUE %0, "x", (DW_OP_LLVM_fragment 0 8) ; DV3
  |
  DV1(%0)
  DV2(%1) <----| %0 is not used in this interval in this map.
  DV3(%0)      | Really, DV2 should not be in this map.

Because the spill range isn't represented in the interval map we just lose that
coverage.

Patch D66415 <https://reviews.llvm.org/D66415> changes this so that the DBG_VALUEs are filtered into interval maps
based on { Source Variable, Fragment } and will coalesce adjacent entries that
use the same { Location, DIExpression }.

This method correctly handles fully overlapping fragments and non-overlapping
fragments of the same Source Variable and fixes case [0] and [1]. However, it
still produces incorrect debuginfo when presented with partially overlapping
fragments in much the same way:

  Example 10
  +-------------------DBG_VALUE %0, "x", (DW_OP_LLVM_fragment 0 16) ; DV1
  |                   ...
  |              +----DBG_VALUE %1, "x", (DW_OP_LLVM_fragment 0 8)  ; DV2
  |              |    ...
  +--------------|----DBG_VALUE %0, "x", (DW_OP_LLVM_fragment 0 16) ; DV3
  |              |
  DV1(%0, Expr0) DV2(%1, Expr1)
  DV3(%0, Expr0)

DV2 will get filtered into a separate interval map and therefore DV3 and DV1
will be incorrectly coalesced.

So, the patch D66415 <https://reviews.llvm.org/D66415> may cause incorrect debuginfo, but in practice it is should
cause much less than the current implementation because we are more building more
accurate intervals for any given bits.

> ^ Sorry, I didn't quite parse this definition.

TL;DR The upper bound of DBG_VALUEs coming into LDV with partially overlapping
fragments in a clang 3.4 RelWithDebInfo build is around 0.00005%.

Summary and questions
---------------------

Partially overlapping fragments (between DBG_VALUEs) are allowed in IR and MIR
but they are not handled everywhere.  For a large C++ codebase I saw a
negligible number of partial overlaps coming into LiveDebugVariablesq.

This patch (in theory) fixes the problem of overlapping fragments for LDV. But
it is large and difficult to review, so:

1. Please can we accept a patch that looks like D66415 <https://reviews.llvm.org/D66415> (plus FIXME: overlaps)? It will produce incorrect debuginfo given partially overlapping fragments, but it is still strictly an improvement on the current implementation, fixes [0] and [1], and is far simpler than this patch which does handle partial overlaps.

My numbers are only anecdotal and there may be languages / situations which
necessitate partially overlapping fragments (e.g. C allows much more type
punning than C++). With this in mind:

2. It seems difficult to actually get clang to produce these pesky overlaps. Do you know of any cases where overlapping fragments in either IR or MIR are _required_ to produce correct debuginfo? Hypothetically, if it were feasible to ban partial overlaps, then D66415 <https://reviews.llvm.org/D66415> would be fully correct and it would be very easy to make D71478 <https://reviews.llvm.org/D71478> optimal.

Thanks,
Orlando

[0] https://bugs.llvm.org/show_bug.cgi?id=41992
[1] https://bugs.llvm.org/show_bug.cgi?id=43957

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

https://reviews.llvm.org/D70121