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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 11 inline comments as done.
labath added a comment.

Thanks for the review Greg. See my responses inline. I'm going to try incorporating the changes tomorrow.



================
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) {
----------------
clayborg wrote:
> Do we need to iterate over the file multiple times here? We do it once here, and then once on line 260. 
The two loops iterate over different parts of the file. The first one goes through the FILE records, and this one does the FUNC records. So the iteration here is efficient because we already know where different kinds of records are located in the file.

(of course, to figure out where these records are located, we've had to go through it once already (in ObjectFileBreakpad), so we still have to make two passes over this data in general. However, that is pretty much unavoidable if we want to do lazy (i.e. random access) into the file as it doesn't have any kind of index to start with.)


================
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;
----------------
clayborg wrote:
> 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?
I think I could avoid creating the CompileUnit object here. However, I will still need to do the parsing here, as I will need to figure out the number of compile units first (best I might be able to achieve is to delay this until GetNumCompileUnits() time).


================
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());
----------------
clayborg wrote:
> 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
That is pretty much what happens here. CompUnitData construct the line table (almost) lazily. It doesn't preparse. The reason I have this indirection, is that the creation of line tables is coupled with the creation of the support file list:
- in order to build the line table, I (obviously) need to go through the LINE records
- however, I also need to go through the LINE records in order to build the CU file list, because I need to know what files are actually used in this "CU"

It seemed like a good idea to me to avoid parsing the LINE records twice. So what I've done is that on the first call to (ReleaseLineTable|ReleaseSupportFiles), CompUnitData will parse both things. Then, the second call will return the already parsed data. That seems like a good tradeoff to me as these two items are generally used together (one is fairly useless without the other). 


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h:151-154
+  struct Bookmark {
+    uint32_t section;
+    size_t offset;
+  };
----------------
clayborg wrote:
> Why do we need more than just a file character offset here?
That's because ObjectFileBreakpad breaks down the file into sections (so all FILE records would go into one section, PUBLIC records into another, etc.). This means that we don't need any "bookmarks" when we want to jump straight to the PUBLIC records for instance, but it does mean we need two coordinates (section, offset) when we want to jump to a specific record within a section.


================
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;
+    }
----------------
clayborg wrote:
> 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?
I'd like the keep to enable the CompUnitData for conjugated parsing of line tables and support files. I think I can get rid of the compile unit field inside it, but that would mean relying on the SymbolVendor to conjure up the CompUnitSP when I need it, which is a bit of an odd dependency. (I need to conjure up the pointer from somewhere in the ResolveSymbolContext functions).


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h:214
+
+  std::vector<FileSpec> m_files;
+  CompUnitMap m_comp_units;
----------------
clayborg wrote:
> Use FileSpecList?
FileSpecList doesn't have the `resize` method. I use that to implement parsing of the FILE records, since the file records theoretically don't have to come in order, so I just resize the vector to fit the largest number I've encountered.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56595/new/

https://reviews.llvm.org/D56595