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

Fri Dec 2 06:33:29 PST 2022

labath added a comment.

I don't believe there's no way to split this patch up. I mean, just half of it is dedicated to changing PRIx32 into PRIx64. Surely that can be a patch of it's own, even if it meant that DWARFDIE::GetOffset temporarily returns something other than dw_offset_t. And if we first changed that to use the llvm::formatv function (which does not require width specifiers), then changing the size of dw_offset_t would be a no-op there. And the fact that the patch description can be summed up into one sentence (enable 64-bit offsets) means that I as a reviewer have to reverse-engineer from scratch what all of these (very subtle) changes do and how they interact.

For example, I only now realized that this patch makes the DIERef class essentially interchangable with the `user_id_t` type (in has an (implicit!) constructor and a get_id() function). That's not necessarily bad, if every DIERef can really be converted to a user_id_t. However, if that's the case, then why does `SymbolFileDWARF::GetUID` still exist?

Previously the DIERef class did not encode information about which "OSO" DWARF file in the debug map it is referring to, and they way that was enforced was by having all conversions go through that function (which was the only way to convert from one to the other). If every DIERef really does carry the OSO information then this function (and it's counterpart, `DecodeUID`) should not exist. If it doesn't carry that information, then we're losing some type safety because we now have two ways to do the conversion, and one of them is (sometimes?) incorrect. Maybe there's no way to avoid that, it's definitely worth discussing, and it that would be a lot easier without the other changes in the way.

As for the discussion, I am still undecided about whether encoding the OSO information into the DIERef is a good thing. In some ways, it is very similar to dwo files (whose information is encoded there), but OTOH, they are also very different. An OSO is essentially a completely self-contained dwarf file, and we represent it in lldb as such (with its own Module, SymbolFile objects, etc.). A DWO file is only syntactically independent (e.g. its DIE tree can be parsed independently), but there's no way to interpret the information inside it without accessing the parent object file (as that contains all the address information). This is also reflected in how they're represented in LLDB. The don't have their own Module objects, and the SymbolFileDWARFDwo class is just a very thin wrapper that forwards everything to the real symbol file. Therefore, it does not seem *un*reasonable to have one way/object to reference a DIE inside a single SymbolFileDWARF (and all the DWO files it references), and another to reference *any* DIE in the set of all SymbolFileDWARFs (either a single object, or multiple objects managed by a SymbolFileDWARFDebugMap) which provide the information for this module.

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:52
+
+  DIERef(lldb::user_id_t uid) {
+    m_die_offset = uid & k_die_offset_mask;
----------------
At least make this explicit so it can't be constructed from any random integer. I'd consider even making this a named (static) function (e.g. `DIERef fromUID(user_id_t)`), as one should be extra careful around these conversions.

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:16

+#include "DIERef.h"
 #include "lldb/Core/Module.h"
----------------
This would look much better in the block on line 60, next to the other includes from this directory.
Or, even better, if you just delete all the empty lines between the includes, then clang-format will automatically sort the whole thing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618