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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think that the main reason we've arrived at such different conclusions is that I treat the "user IDs of DIEs" and and "user IDs of symbol files" as essentially two different things (namespaces if you will). Obviously, one needs the symbol file ID in order to compute the DIERef ID, but theoretically those two can use completely different encodings. With this take on things, I stand by my assertion that DIERef->user_id conversions are tightly controlled. The symbol file ID computations are a mess.

You, if I understand correctly, see the ID of a symbol file as a special case of an all-encompassing user id -- essentially a user_id (or a DIERef) pointing to the first byte of the symbol file. with this world view, the entirety of user ID computation is a mess. :)
I can definitely see the appeal of viewing the world that way. It's nice and uniform and unambiguous (since you can't have a DIE at offset zero) -- it's just not the view I had when I was writing this code a couple of years ago. :) And it has the disadvantage of obscuring the DIERef->user_id transition (for DIEs at least), and that's what I'm weight against the other advantages of that approach.

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

> I guess main issue with GetUID is that we rely on an internal state of SymbolFileDWARF to
>
> 1. figure out if we are dealing with dwo or oso with check for GetDebugMapSymfile
> 2. get extra data GetDwoNum(), and GetID()
>
> We can either push that logic on the caller side of things (not I deal I would think) and remove GetUID, or extend the constructor to be a bit more explicit. This way all the bit settings are still consolidated, but the logic of when to create what is still hidden in GetDebugMapSymfile.
>
> What do you think?

I'm not entirely sure what you mean by that, but I think either of those could be fine. Essentally, what I'm trying to achieve is to make sure is that if the DIERef<->user_id conversion is trivial, then it is always valid to perform it (i.e. there are no partially constructed DIERefs). Ideally, there wouldn't be partially constructed DIERefs in any case, but that is not as important if one is forced to provide that additional information in order to do the conversion.

However, I also want to throw out this alternative <https://reviews.llvm.org/D140298>. This one goes in the completely opposite direction. Instead of centralizing the conversions, it federates it (which is I think is roughly what I had in mind when I worked on this last time). There is no single place which controls the conversion, but there are multiple **disjoint** places which do that:

- one for the OSO case. This includes the following problematic lines you've listed:

> GetOSOIndexFromUserID
> GetUID (1/2)
> Encode/Decode UID (1/2)
> return DecodedUID{
>
>   *dwarf, {std::nullopt, DIERef::Section::DebugInfo, dw_offset_t(uid)}};

- one for the DWO case:

> GetUID (1/2)
> Encode/Decode UID (1/2)

- one for Symbol File IDs (which is does a +1 on the internal index -- bacause the main object file has ID 0)

> oso_symfile->SetID(((uint64_t)m_cu_idx + 1ull) << 32ull);
> SymbolFileDWARFDwo constructor
> GetDwoNum (cancels out the previous one)

And I don't think it's an obstacle for making the die offsets larger -- I've included comments on how I think that could happen.

It doesn't handle this one, which seems just wrong, and should be made to use GetUID/DecodeUID

> const dw_offset_t function_die_offset = func.GetID();


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618