[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)

[Pavel Labath via lldb-commits]

labath wrote:

> This patch as-is is NFC?
NFC**I**, I would say :) I don't think this should change the behavior in any way, but it's pretty hard to guarantee that.

> (no tests) but without this patch, the other delay-definition-die patch would have had a problem?
> 
> Is it possible to add a test in this patch that would exercise the thing that would become buggy if the delay-definition-die patch were to be recommitted?

Sort of. The situation is a bit complicated, because this touches pretty much the same code as the other patch, so the other patch will not apply cleanly or become magically correct. It's more like a building block that enables us to rewrite the other patch in a more robust manner -- which brings us to the second way this is complicated: It's not that the other patch was wrong on its own. It was just very sensitive to the other bugs. Previously, if we failed to unique the types correctly, we would "just" get the wrong type (or maybe no type). With the patch, that situation would trigger a hard assert. On its own, that might even be considered a good thing (easier to find bugs), we're it not for the fact that this made the logic of the patch very hard to follow. So, this is my attempt to make it more straight-forward.

As for tests, it is possible to write a test which would reproduce a crash with the original patch, but that test is contingent on the existence of another bug. When I reverted that patch, I added a test (in de3f1b6d68ab8a0e827db84b328803857a4f60df) which triggered the crash. However, now that that bug is fixed (#95905), it does not crash anymore. Now, I happen to know of another bug (which happens to be triggered by the same code, only compiled with type units), but the same thing will happen once that bug is fixed. Given all of that, I don't think another test case is needed with this particular patch. It might be interesting for the final delay patch, if we don't fix the type unit thing by then, but I think of this patch mostly as a cleanup.

https://github.com/llvm/llvm-project/pull/96484