ayermolo wrote: > 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.
BOLT does as I mentioned in: https://github.com/llvm/llvm-project/pull/87740#issuecomment-2060023287 https://github.com/llvm/llvm-project/pull/87740 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
