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

Wed Jan 11 01:47:42 PST 2023

StephenTozer added a comment.

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 was added to implement 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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133925