clayborg added inline comments.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:22-24
+/// - file_index: identifies the dwo file in the Module. If this field is not
+/// set,
+/// the DIERef references the main, dwo or .o file.
----------------
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:32
enum Section : uint8_t { DebugInfo, DebugTypes };
-
- DIERef(std::optional<uint32_t> dwo_num, Section section,
+ // Making constructors protected to limit where DIERef is used directly.
+ DIERef(std::optional<uint32_t> file_index, Section section,
----------------
labath wrote:
> they're not actually protected
There were for a bit to control access to this, but in reality we ended up
friending too many classes and then ran into issues with the DIERefTest stuff,
so we decided to make them public again. We can remove this comment.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp:75
+ if (ref)
+ return DIERef(*GetDWARF(), *ref).get_id();
+
----------------
labath wrote:
> Is this the only call site of the `DIERef(SymbolFileDWARF&)` constructor?
>
> If so, and if we make it such that `DWARFBaseDIE::GetDIERef` returns the
> fully filled in DIERef, then this function can just call get_id() on the
> result, and we can delete that constructor.
This line doesn't make sense. If we got a valid DIERef back from GetDIERef(),
then we just return that as it would have used the SymbolFileDWARF to fill
everything in already. So we might not need that extra constructor if this is
the only place as Pavel suggested.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1682
SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
- if (die_ref.dwo_num()) {
- SymbolFileDWARF *dwarf = *die_ref.dwo_num() == 0x3fffffff
- ? m_dwp_symfile.get()
- : this->DebugInfo()
- .GetUnitAtIndex(*die_ref.dwo_num())
- ->GetDwoSymbolFile();
- return dwarf->DebugInfo().GetDIE(die_ref);
- }
-
- return DebugInfo().GetDIE(die_ref);
+ return GetDIE(die_ref.get_id());
}
----------------
labath wrote:
> clayborg wrote:
> > Pavel: note we now really on "SymbolFileDWARF::GetDie(user_id_t)" to be the
> > one source of truth when finding a DIE. We could make
> > "SymbolFileDWARF:GetDie(DIERef ref)" be the one source of truth and then
> > have "SymbolFileDWARF::GetDie(user_id_t)" just create a local DIERef and
> > then call "SymbolFileDWARF:GetDie(DIERef ref)" if that would be cleaner.
> +1
Ok. So lets do this - change "DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t
uid)" to just be:
```
DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t uid) {
return GetDIE(DIERef(uid));
}
```
And then change the current "DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t
uid)" to be the one that does all of the work:
```
DWARFDIE SymbolFileDWARF::GetDIE(DIERef die_ref) {
std::optional<uint32_t> file_index = die_ref.file_index();
if (file_index) {
if (SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile())
symbol_file = debug_map->GetSymbolFileByOSOIndex(*file_index); // OSO case
else if (*file_index == DIERef::k_file_index_mask)
symbol_file = m_dwp_symfile.get(); // DWP case
else
symbol_file = this->DebugInfo()
.GetUnitAtIndex(*die_ref.file_index())
->GetDwoSymbolFile(); // DWO case
} else if (die_ref.die_offset() == DW_INVALID_OFFSET) {
symbol_file = nullptr;
} else {
symbol_file = this;
}
if (symbol_file)
return symbol_file->GetDIE(die_ref);
return DWARFDIE();
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138618/new/
https://reviews.llvm.org/D138618
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
