This is an archive of the discontinued LLVM Phabricator instance.

GSYM: Add the llvm::gsym::Header header class with tests
ClosedPublic

Authored by clayborg on Sep 17 2019, 9:23 AM.

Details

Summary

This is another patch that originated from the full GSYM patch https://reviews.llvm.org/D53379.

This patch adds the llvm::gsym::Header class which appears at the start of a stand alone GSYM file, or in the first bytes of the GSYM data in a GSYM section within a file. Added encode and decode methods with full error handling and full tests.

Diff Detail

Repository
rL LLVM

Event Timeline

clayborg created this revision.Sep 17 2019, 9:23 AM
JDevlieghere accepted this revision.Sep 17 2019, 9:38 AM

LGTM!

include/llvm/DebugInfo/GSYM/Header.h
90 ↗(On Diff #220518)

Any reason you prefer isValid over operator bool?

This revision is now accepted and ready to land.Sep 17 2019, 9:38 AM
aprantl added inline comments.Sep 17 2019, 10:03 AM
include/llvm/DebugInfo/GSYM/Header.h
90 ↗(On Diff #220518)

As usual, I'm going to point out that it may be nicer to have the parser return an Expected<Header> instead of a header that may be invalid.

clayborg marked 2 inline comments as done.Sep 17 2019, 10:32 AM
clayborg added inline comments.
include/llvm/DebugInfo/GSYM/Header.h
90 ↗(On Diff #220518)

This is because people will create a Header using information retrieved from another source, like DWARF, breakpad or compiler metadata. Once the header has been populated, it might get emitted. Also, for native endianness, we won't be parsing these, we will just be mmap'ing the data and getting a pointer to the start of the GSYM data. In this case, we will call this function to ensure the GSYM header is good. Note that the encode and decode functions do the right thing. This function will be used by clients that mmap and try to use the data as is.

90 ↗(On Diff #220518)

Any reason you prefer isValid over operator bool?

Mostly in case this is used internally inside the class. I hate seeing:

if (*this)

I would rather see:

if (isValid())
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2019, 10:45 AM