[Lldb-commits] [PATCH] D56595: SymbolFileBreakpad: Add line table support

[Greg Clayton via Phabricator via lldb-commits]

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