[Lldb-commits] [PATCH] D73206: `DWARFASTParserClang::m_decl_ctx_to_die` `DWARFDIE`->`DIERef` for DWZ

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It's been a while since we had that discussion, but the way I'm remembering it, sizeof(DWARFDIE) was not my biggest concern with this stuff (though I may have been echoing the concerns of some other people). What I was trying to prevent is diverging even more from the llvm dwarf implementation. In that sense, changing lldb's DWARFDIE without a matching change to the llvm counterpart is equally bad. I don't think the first two options are ever going to make it into llvm. The third might, but even that may be a hard sell -- a slightly more tractable option might be to reuse the low-order bits in the DWARFDIE via PointerIntPair. We might be able to get those eight bits that way.

But all of that seems peripheral to me. The main thing I was trying to achieve was to have the low-level DWARF code (DWARFUnit, DWARFDIE, ... -- basically the things that are in the llvm dwarf parser right now) not require access to anything outside of what is available to them. If there's a thing which requires more info, it should be built on top of this lower layer, not trying to reach out from inside it. Once it's outside, a lot of options become available. If there is a lldb_private::CompileUnit in the vicinity of this code, it could just fetch the language from there. Or, we can have some sort of a SuperDWARFDIE, which includes the language, or the main compile unit (either lldb or dwarf) to pass around as a single entity (using PointerIntPair would be an optimization of that). Or, instead of SuperDWARFDIE we could use a DIERef, a user_id_t, or some of the other reference kinds we have already..

As for this patch, I don't see any big problem with changing the representation in this case, but given that it's motivation is to able to  later change DWARFDIE, I don't think its really useful/needed.

The test changes are cool, and I'd encourage you to split them off into a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73206