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

Fri Jan 27 05:07:38 PST 2023

labath added a comment.

In D138618#4083481 <https://reviews.llvm.org/D138618#4083481>, @clayborg wrote:

> We just need to create all DIERef objects using the GetID() from the symbol file as the file index, and we should be able to remove the SymbolFile::GetUID() function now. As long as file index zero is reserved for "vanilla DWARF that doesn't use DWO or OSO we will know the difference. We might want to not have SymbolFileDWARF inherit from UserID at all, and switch over to have SymbolFileDWARF add a virtual function:
>
>   uint32_t m_file_index = 0; // Zero means main DWARF file, 1...N identifies the Nth DWO file or OSO file
>   virtual uint32_t GetFileIndex() { return m_file_index; }
>
> Then anyone can set the file index correctly for DWO or OSO files. And we avoid using user_id_t values for the symbol files since they aren't needed.

This isn't about the "user id" of a symbol file. I'm totally happy with the changes there -- though I also wouldn't be opposed to changing the "user id" field to something more explicit (like the file index).

My problem is with the "user id"s of individual DIEs. Currently, if I have a DWARFDIE, the only way to get its user id is to do something like `die.GetDWARF()->GetUID(die)`. With this patch, there are two ways:

1. the same as before
2. `die.GetDIERef()->get_id()`

The problem is that the second way is not going to be correct for OSO files because that path will not set the "oso" component of the DIERef. The worst part is that the second method is much shorter than the first one, so I think it will be very tempting to use it -- and it will actually be right most of the time, until that code is used in an OSO context.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618