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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
