[PATCH] D133925: [DebugInfo] Fix: Variables that have no non-empty values being emitted when they have a DBG_VALUE_LIST

Wed Jan 11 16:03:37 PST 2023

dblaikie added a comment.

In D133925#4042770 <https://reviews.llvm.org/D133925#4042770>, @StephenTozer wrote:

> In D133925#4040811 <https://reviews.llvm.org/D133925#4040811>, @dblaikie wrote:
>
>> I'll see if I can come up with a reproducer. Do you have an example of this from clang, rather than from handcrafted IR?
>
> The problem fixed by this was found while building in the LLVM test suite - I can find an organic reduced reproducer, but I think it's worth noting that this case is quite simple and could easily show up in any optimized debug info build, the basis for this patch is as follows: `DbgEntityHistoryCalculator` skips recording entries for variables that have no non-empty locations within their scope; prior to this patch, it recognized undef `DBG_VALUE`s as being undef, but did not recognize undef `DBG_VALUE_LIST`s as being undef, meaning that in any scenario where the `DbgEntityHistoryCalculator` would be responsible for determining whether a variable should be emitted or omitted, an undef `DBG_VALUE` would give a different output to an undef `DBG_VALUE_LIST`.
>
>> So, at least on the test update itself, I'd say this patch creates a bug - by dropping the local variable where it was previously correctly described as existing but without a location.
>
> It is //possible// that this patch creates a bug by exposing an issue that exists elsewhere - I'd much prefer tackling that issue directly rather than reverting this patch however, as there is a fairly substantive patch stack[0] that has landed since this patch that relies on variadic and non-variadic debug values being treated the same as each other, and in particular on the assumption that undef `DBG_VALUE`s and `DBG_VALUE_LIST`s have identical meanings.
>
> I think some investigation is warranted for the dropping of variables, but the dropping of the variable in this test case is due to the fact that I manually added the variable but did not add it to the retained nodes list. For us to be sure that there is a bug, we would need to see an organic example of a variable being dropped, and confirm that it is being dropped incorrectly. I mentioned before that there was a discussion about this, which my esteemed colleagues have helped me find[1], in which it was determined that there was a legitimate case for omitting variables in an inlined function with no `DW_AT_location`.
>
> After reviewing that thread, I believe that insofar as this patch is responsible for changing the variable counts, it is most likely correct in doing so. The function that this patch modifies, `hasNonEmptyLocation`, is used explicitly for determining when it is desirable to drop a variable: when the variable has no `DW_AT_location`, and it is inlined, such that the variable entry contains only a `DW_AT_abstract_origin`. The function that this patch modifies is the function that was added by the patch[2] that implements the agreed-upon fix from that discussion - essentially, the fix in that patch is one for which there is a general agreement that it is doing the right thing, and this patch simply fixes an error in //that// patch, which was written without reference to variadic debug values.
>
> In summary, I don't think there is a good reason right now to assume that the patch is causing variables to be erroneously dropped - if there is a reproducer or some other indication that variables are being incorrectly omitted then I'm happy to look into it; I'm also happy to update the test to insert `localh` into the retained nodes list, so that it is not dropped anymore.
>
> [0] https://reviews.llvm.org/D129372
> [1] https://lists.llvm.org/pipermail/llvm-dev/2021-January/148139.html
> [2] https://reviews.llvm.org/D95617

Yeah, thanks for all the context - from what I've seen so far in looking cases like https://groups.google.com/g/llvm-dev/c/qtlVVEK6gso/m/mEn3c1fFAwAJ?pli=1 seem plausible, it'd be great to get that `llvm-dwarfdump --statistics` bug fixed, so it counts variables even when they're only present in the abstract origin.

Though the size reports I was looking at included significant (3-4%) reduction in .debug_loclists size, which doesn't track with this hypothesis (if it were only trivial inlined variables with an abstract_origin and no location being removed (in favor of only having an abstract origin and no concrete version) then loclists shouldn't've changed size) - but it's possible the loclist changes were due to other source change- I don't think those have been tested on this commit alone. I'll try doing that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133925