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

Tue Jan 17 16:04:32 PST 2023

clayborg added a comment.

In D138618#4004866 <https://reviews.llvm.org/D138618#4004866>, @labath wrote:

> 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.

FWIW: User IDs of symbol files is not part of any API. It was added to SymbolFileDWARF to allow us to identify .o files for mac without dSYM and was used by the fission code that Pavel wrote as well. Since this is internal, it doesn't matter at all how we make or use the IDs. No public interface will ever expect a SymboFile to have a lldb::user_id_t. Therefore it is just down to how to use these IDs within the DWARF symbol plug-ins for both mac with .o files and for fission with .dwo files.

Things that are created by the DWARF, like lldb_private::CompileUnit, lldb_private::Function, lldb_private::Block, and lldb_private::Type do have lldb::user_id_t and they are expected to make IDs that make sense for the individual SymbolFile plug-in to be able to easily match up with something in the DWARF. All of these objects have DIEs in the DWARF, so we must be able to make a lldb::user_id_t that allows us to easily answer more questions about something at a later date in the DWARF. Like we can make a lldb_private::CompileUnit without making any functions, blocks or types. If we are later asked to find all functions for a compile unit, we should be able to take the lldb::user_id_t of the compile unit and easily do this.

So how DIERef is used is solely up to the DWARF symbol file plug-in.

So we could just assign the user IDs of each SymbolFileDWARF to be the index of the .o file for mac, or the index of the .dwo file for fission. It doesn't really matter. As long as we can easily take a user_id_t from a virtual interface and track it back to the DIE we care about.

> 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();

yes this is wrong and should be changed. It only used to work because we knew that the bottom 32 bits of a a 64 bit user_id_t was the DIE offset, which isn't true anymore.

Since the user IDs of SymbolFileDWARF plug-ins mean nothing to anyone else, we can make them what we need them to be so they work for us. I would suggest to remove the use of DIERef from calculating the IDs of symbol files and have .o files for mac and .dwo files for fission use a 1 based index as their ID to make it easy to encode into a DIERef when needed for lldb::user_id_t values that _are_ included in objects that we hand out. Is there anything else that would need to be done to keep everyone happy here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618