labath marked 8 inline comments as done.
labath added inline comments.
================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.h:30
- DIERef(Section s, llvm::Optional<dw_offset_t> u, dw_offset_t d)
- : m_section(s), m_unit_offset(u.getValueOr(DW_INVALID_OFFSET)),
- m_die_offset(d) {}
-
- Section section() const { return static_cast<Section>(m_section); }
+ DIERef(llvm::Optional<uint32_t> num, Section s, dw_offset_t d)
+ : m_dwo_num(num.getValueOr(invalid_dwo_num)), m_section(s),
----------------
clayborg wrote:
> maybe make "llvm::Optional<uint32_t> num" the last arg and give it a
> llvm::None default argument value? Rename to dwo_num as well.
I'd rather not do that. There aren't that many places that are constructing
DIERefs -- I counted just four, and only one (the one in apple index) of them
is passing llvm::None as the argument. I think it's better to have the person
writing the code stop and think about what is the right value here, instead of
just hoping that the default argument will be correct (which it won't be, in
most cases).
Also having the arguments ordered this way makes it a nice logical progression.
First you select the file, then the section in that file, and finally you give
the offset in that section.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFIndex.h:59
/// vector.
- void ProcessFunctionDIE(llvm::StringRef name, DIERef ref,
- DWARFDebugInfo &info,
+ void ProcessFunctionDIE(llvm::StringRef name, const DIERef &ref,
+ SymbolFileDWARF &dwarf,
----------------
clayborg wrote:
> can/should we still pass "const DIERef &ref" by value like we used to now
> that it is smaller?
Yeah, that's a good point.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1224-1230
+ DIERef::Section section =
+ uid >> 63 ? DIERef::Section::DebugTypes : DIERef::Section::DebugInfo;
+
+ llvm::Optional<uint32_t> dwo_num = uid >> 32 & 0x7fffffff;
+ if (*dwo_num == 0x7fffffff)
+ dwo_num = llvm::None;
+
----------------
clayborg wrote:
> Can we move the lldb::user_id_t code into DIERef now that it is really simple
> to decode? Too many details are leaking out of DIERef like its invalid dwo
> num enocding, where the section is in the user_id_t...
>
> This code could be:
> ```
> return DecodedUID{*this, DIERef(uid)};
> ```
>
I don't think that's a good idea for two reasons:
- decoding is fairly simply in the non-debug-map case, but with a debug-map,
the decoding is still relatively complicated, as you need to consult the debug
map in order to find the right symbol file. If we don't move that part into the
DIERef class, then we'll end up with a constructor that only works in some
cases, which will be confusing. If we do move it, then we're letting
information about SymbolFileDWARF and it's relationship to
SymbolFileDWARFDebugMap leak into DIERef class.
- I wouldn't even say this is leaking information out of the DIERef class.
SymbolFileDWARF is responsible for the conversion from a DIERef into a
user_id_t (in SymbolFileDWARF::GetUID), and so it should also covert it back.
For that to work, it obviously needs to be aware of the fields in the DIERef
and their value ranges, but it is free to encode that information into a
user_id_t any way it sees fit. (However, this reminds me that I should update
GetUID to make use of the dwo_num abstraction. Right now, it works because the
dwo_num is derived from the SymbolFile ID, but it would be cleaner to use
dwo_num explicitly.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63428/new/
https://reviews.llvm.org/D63428
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
