Page MenuHomePhabricator

Breakpad: Extract parsing code into a separate file
ClosedPublic

Authored by labath on Jan 17 2019, 4:06 AM.

Details

Summary

This centralizes parsing of breakpad records, which was previously
spread out over ObjectFileBreakpad and SymbolFileBreakpad.

For each record type X there is a separate breakpad::XRecord class, and
an associated parse function. The classes just store the information in
the breakpad records in a more accessible form. It is up to the users to
determine what to do with that data.

This separation also made it possible to write some targeted tests for
the parsing code, which was previously unaccessible, so I write a couple
of those too.

Event Timeline

labath created this revision.Jan 17 2019, 4:06 AM

Are we missing parsing of FUNC and line entry lines in this patch?

clayborg accepted this revision.Jan 17 2019, 10:58 AM

Never mind, just read the update to your other patch. Would be nice to identify line records in toToken, but that will probably cost more CPU cycles that it is worth, so I think this is a good start.

This revision is now accepted and ready to land.Jan 17 2019, 10:58 AM
This revision was automatically updated to reflect the committed changes.
lemo added a comment.Jan 18 2019, 11:15 AM

Looks good. A few questions/suggestions inline.

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
74 ↗(On Diff #182485)

the comment is great, but I think we should still have symbolic constants for all these magic values

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
34 ↗(On Diff #182485)

should this be virtual? (even though the class doesn't have other virtual members, the class hierarchy introduces the possibility for consumers to treat them a polymorphic types - ex. storing as Record* and use the kind type to figure out the concrete type)

40 ↗(On Diff #182485)

Just curious, what is the definitive convention for naming data members? A lot of LLDB code uses the m_camelCase convention.

84 ↗(On Diff #182485)

most of these types are just plain data containers, so why not make them structs? (and avoid all the boilerplate associated with public accessors, repetitive constructors, ...)

labath marked 7 inline comments as done.Jan 21 2019, 12:34 PM

Thanks for the review. I'll create another review with the changes stemming from this.

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
74 ↗(On Diff #182485)

I just had an idea on how to remove the numbers altogether. I'll create a separate patch to see what you think.

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
34 ↗(On Diff #182485)

Yes, but they won't be able to polymorphically delete these objects through a base pointer, as the base destructor is protected. With a protected destructor, the compiler wouldn't warn here even if the class had other virtual members.

(BTW, this is one of the reasons I gave up on a generic "parse" function - it'd force heap allocations for polymorphic treatment without any big benefit).

40 ↗(On Diff #182485)

There isn't one. :/

The reason I used the llvm naming convention here, is because this was a fresh class, which didn't have to interact with lldb at all. That's why I could (and wanted to) follow llvm coding practices as best as I could. Using lldb names for things felt weird here.

84 ↗(On Diff #182485)

I thought about that too, but I couldn't really make up my mind. In the end I did this, but I can't say I had a good reason for it. I think I can change the members to be public, given that you had the same thought.