clayborg added a comment.
I like the way you did the compile units and the line tables and support file
list. It would be nice to change this to do things more lazily. Right now we
are parsing all compile unit data into CompUnitData structures and then passing
their info along to the lldb_private::CompileUnit when we need to. We can be
more lazy, see inlined comments.
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:279
+ llvm::Optional<FuncRecord> func;
+ for (LineIterator It(*m_obj_file, Record::Func), End(*m_obj_file); It != End;
+ ++It) {
----------------
Do we need to iterate over the file multiple times here? We do it once here,
and then once on line 260.
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:281-300
+ if (auto record = FuncRecord::parse(*It)) {
+ id = It.GetBookmark();
+ func = record;
+ } else if (!id)
+ continue;
+ else if (auto record = LineRecord::parse(*It)) {
+ FileSpec spec;
----------------
Seems like we should just populate the m_compile_units data with address range
to character file offset here? When we are asked to create a compile unit, we
do this work by going to the "lldb_private::CompileUnit::GetID()" which will
return the file offset and we just head there and start parsing?
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:314
CompUnitSP SymbolFileBreakpad::ParseCompileUnitAtIndex(uint32_t index) {
+ CompUnitSP cu = m_comp_units.GetEntryRef(index).data.GetCompUnit();
----------------
This seems like where we would do the heavy parsing code that are in the
initialize object function. .Get the character file offset from m_compile_units
and just parse what we need here? It will cause less work for us in the
initialize object call then and we can be lazier
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:326
bool SymbolFileBreakpad::ParseLineTable(CompileUnit &comp_unit) {
- // TODO
- return 0;
+ CompUnitData &data = m_comp_units.GetEntryRef(comp_unit.GetID()).data;
+ comp_unit.SetLineTable(data.ReleaseLineTable(*m_obj_file,
m_files).release());
----------------
From the compile unit, if GetID() returns the character file offset to the FUNC
or first LINE, then we don't need the preparsed CompUnitData? We can just parse
the line table here if and only if we need to
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:332
+ FileSpecList &support_files) {
+ CompUnitData &data = m_comp_units.GetEntryRef(comp_unit.GetID()).data;
+ support_files = data.ReleaseSupportFiles(*m_obj_file, m_files);
----------------
Ditto above
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h:151-154
+ struct Bookmark {
+ uint32_t section;
+ size_t offset;
+ };
----------------
Why do we need more than just a file character offset here?
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h:182-188
+ CompUnitData &operator=(const CompUnitData &rhs) {
+ m_comp_unit = rhs.m_comp_unit;
+ m_bookmark = rhs.m_bookmark;
+ m_support_files.reset();
+ m_line_table_up.reset();
+ return *this;
+ }
----------------
Seems like if we just pick the file character offset of the function or the
function's line entries as the lldb_private::CompileUnit user ID
(lldb::user_id_t) then we don't really need this class? We just create the
compile unit as a lldb_private::CompileUnit and our symbol file parser will
fill in the rest? Any reason we need this CompUnitData class?
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h:212
+
+ using CompUnitMap = RangeDataVector<lldb::addr_t, lldb::addr_t,
CompUnitData>;
+
----------------
Could this just be:
```
using CompUnitMap = RangeDataVector<lldb::addr_t, lldb::addr_t, lldb::offset_t>;
```
Where offset_t is the character fie offset for the first line of the FUNC
entry? Any reason to use CompUnitData instead of just creating
lldb_private::CompileUnit objects?
================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h:214
+
+ std::vector<FileSpec> m_files;
+ CompUnitMap m_comp_units;
----------------
Use FileSpecList?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56595/new/
https://reviews.llvm.org/D56595
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
