[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

https://github.com/labath commented:

Most of the patch is very clean, but I'm bothered by the `getForeignTUSkeletonCUOffset` function, how it opens up with a mostly redundant (the callers checks this already) call to `getForeignTUTypeSignature`, and then proceeds with a reimplementation of `getCUOffset` (sans the type unit check). I think we could design a better set of APIs for this, but I'm not sure what those is, because I'm unclear of the meaning of various combinations of DW_IDX unit entries.

What the new function (I think) essentially does is "give me the CU associated with this entry even if the entry does not describe a DIE in this CU" (because `DW_IDX_die_offset` is relative to a type unit)". I think this API would make sense, even without needing to talk about type units. However, is it actually implementable? For single-CU indexes, that's fine, because we can kind of assume all entries are related to that CU. But what about multi-CU indexes? The only way to determine the CU there is to have an explicit attribute. Are we saying that each entry in a multi-CU index must have a DW_IDX_compile_unit (in addition to a DW_IDX_type_unit)?

If the answer is yes, then this function can be implemented, but then I think the current implementation of `getCUOffset` (which I interpret as "give me the CU offset **IF** this entry is relative to that CU") doesn't make sense -- because it treats entries with an explicit `DW_IDX_compile_unit` different from an implicit/missing `DW_IDX_compile_unit`. And in this world, we assume that `DW_IDX_type_unit` takes precedence over `DW_IDX_compile_unit` -- if both are present then `DW_IDX_die_offset` is relative to the former. And I'm not sure if we would actually want to take up space by putting both values into the index. Looking at existing producers would be interesting, but I'm not sure if there are any (lld does not merge type unit indexes right now, possibly for this very reason). Maybe one could produce something with LTO?

OTOH, if the answer is no, then the function isn't implementable in the generic case. That doesn't mean you can't implement this feature -- which is useful for guarding against ODR violations (exacerbated by llvm's nonconforming type signature computation algorithm...). However, I think it could be implemented in a much simpler way. For example, lldb could just check if it's looking at a single-CU index, and get the CU offset that way. No extra llvm APIs would be needed.

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