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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
