[llvm] Reapply "[llvm/DWARF] Recursively resolve DW_AT_signature references"… (PR #99495)

[Pavel Labath via llvm-commits]

================
@@ -173,7 +169,8 @@ DWARFTypePrinter::appendUnqualifiedNameBefore(DWARFDie D,
   case DW_TAG_base_type:
   */
   default: {
-    const char *NamePtr = dwarf::toString(D.find(DW_AT_name), nullptr);
+    const char *NamePtr =
+        dwarf::toString(D.findRecursively(DW_AT_name), nullptr);
----------------
labath wrote:

> Ah, I guess maybe it'd make sense to split this patch up. (haven't thought about whether these two changes are ordered or order independent)
> 
> The `getAttributeValueAsReferencedDie` accounting for type units is one thing, sounds good.

Sounds good. I'll create a patch for that. In fact, I think lldb already has this part.

> 
> The "recurse through DW_AT_signature" to resolve some lookups is a separate thing - I think it still does make sense... maybe? It is a bit less efficient to have to lookup type units multiple times, compared to the abstract_origin/specification indirection which is at least just jumping to an offset (though I guess it's still a binary search through a list of DIEs, ultimately, to find the DIE with a given offset?).

It is, and I guess that the direct lookup would always be faster, though maybe not it a way that we care.

> I guess alternatively, we could make the children iteration do the same indirection too? Though doing that implicitly might be too much (like even the dumper would get confused, dumping the children of the skeleton type DIE, etc) so then it'd have to be a separate function...

Yeah, that sound like too much. We don't do that for DW_AT_specification either.

> I guess maybe my thoughts are that findRecursively, finding recursively explicitly is OK - but that maybe it doesn't suit the use case here in the type printer (because it's comprehensively visiting lots of stuff - whereas just "give me the name of this thing" or "give me the file/line this thing was defined on" is a relatively more common/mild query that's nice to be able to do more conveniently)

I can definitely understand that position.

> 
> Hmm - when/where/how does /this/ function (`appendUnqualifiedNameBefore`) get called on an unresolved type? Perhaps this should be pushed further up... like every call to "get a DIE from this DW_AT_type" is already looking through to the type unit, what's left/how else does this code end up dealing with a skeleton type unit? (I guess maybe in top level callers - perhaps that needs an explicit entry point that resolves before going into the generic walking code)

I think this can happen anywhere where we have a double reference (so when a DW_AT_type points to a declaration, which points (via DW_AT_signature) to a type unit) (*). `getAttributeValueAsReferencedDie` will only resolve the first reference, and not the second one. Unless we want to teach it to resolve references recursively (which sounds like a bad idea), we're left with explicitly calling some type-unit resolving function at every call site, which sounds a lot like what `resolveReferencedType` already is.

So maybe the verdict is:
- getAttributeValueAsReferencedDie should resolve type unit references
- findRecursively should do that as well
- we should leave type printer alone?

(*) This also means that this problem doesn't occur with gcc-style type unit references, which I'm really starting to like. :)

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