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

[David Blaikie via lldb-commits]

dwblaikie 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.

Sure enough - I take any claim as a statement of intent/belief, not of something mathematically proved, etc.

> > (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 [de3f1b6](https://github.com/llvm/llvm-project/commit/de3f1b6d68ab8a0e827db84b328803857a4f60df)) which triggered the crash. 

Having a bit of a hard time following this - is the test you added the same as the test you speculated is possible  to write in the prior sentence? 

> 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. 

OK - I think I'm following.

> 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.

Perhaps a separate commit could add another RUN line to the existing test you added to demonstrate the reason for the revert? Rather than worrying about which comes first (the type unit patch or the delay patch)

But in any case, I /think/ I understand why this patch doesn't need a test (because this patch avoids the delay patch causing a crash (yeah, more complex than that because the patch doesn't apply cleanly over this one) and that crash already has a test committed) - thanks for the explanation.



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