[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

Wed Jan 25 11:08:30 PST 2023

labath added a comment.

In D138618#4077851 <https://reviews.llvm.org/D138618#4077851>, @ayermolo wrote:

> In D138618#4073329 <https://reviews.llvm.org/D138618#4073329>, @labath wrote:
>
>> I think that the 1-based index thingy helps a lot here, but I haven't seen anything (in your reponse, or in the new patch) that would address my concernt DIERef<->user_id conversion ambiguity. I.e. how is one supposed to know what is the "right" way to convert a DIERef to a user_id:
>>
>> - `die_ref.get_id()`
>> - or `symbol_file.GetUID(die_ref)` (which, funnily enough, will construct another DIERef, and *then* call get_id? (`return DIERef(GetID(), ref.section(), ref.die_offset()).get_id();`)
>>
>> What's your position on that? That we should live with the ambiguity?
>
> Searching for GetUID doesn't look like it's used all that often, maybe follow up patch is just to get rid of it, and replace with DIERef?

If you could make that work, that would be awesome, but I think that's going to be fairly hard. It's true that there aren't that many call sites of this functions, but the ones that are there are very crucial. The user_id_t type represents a symbol-file-neutral identifier (cookie, if you will) that different symbol file implementations use to identify parsed objects (types, mostly). SymbolFileDWARF uses it (via DIERef et al.) to identify the DIE belonging to that type. PDB symbol files use it differently, but the idea is the same. If we wanted to remove that, we'd have to come up with a whole new way to parse/link types -- and one that would work for non-dwarf symbol files as well.

(also, this would not really address my concern, because the question would then become "which DIERef is safe to be used as a Type cookie" (answer: only the one which has the OSO field set), but the reduction in complexity resulting from removing one step from the conversion process (DWARFDIE->DIERef->user_id) might be well worth it.)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138618/new/

https://reviews.llvm.org/D138618