This is an archive of the discontinued LLVM Phabricator instance.

Return llvm::Error and llvm::Expected from some DWARF parsing functions

Authored by zturner on Mar 14 2019, 9:26 AM.



The goal here is to improve our error handling and error recovery while parsing DWARF, while at the same time getting us closer to being able to merge LLDB's DWARF parser with LLVM's. To this end, I've udpated several of the low-level parsing functions in LLDB to return llvm::Error and llvm::Expected.

For now, this only updates LLDB parsing functions and not LLVM. In some ways, this actually gets us *farther* from parity with the two interfaces, because prior to this patch, at least the parsing interfaces were the same (i.e. they all just returned bools, and now with this patch they're diverging). But, I chose to do this for two primary reasons.

  1. LLDB has error logging code engrained deep within some of its parsing functions. We don't want to lose this logging information, but obviously LLVM has no logging mechanism at all. So if we're to merge the interfaces, we have to find a way to still allow LLDB to properly report parsing errors while not having the reporting code be inside of LLVM.
  1. LLDB (and indeed, LLVM) overload the meaning of the false return value from all of these extraction functions to mean both "We reached the null entry at the end of a list of items, therefore everything was successful" as well as "something bad and unrecoverable happened during parsing". So you would have a lot code that would do something like:
while (foo.extract(...)) {

But when the loop stops, why did it stop? Did it stop because it finished parsing, or because there was an error? Because of this, in some cases we don't always know whether it is ok to proceed, or how to proceed, but we were doing it anyway.

In this patch, I solve the second problem by introducing an enumeration called DWARFEnumState which has two values MoreItems and Complete. Both of these indicate success, but the latter indicates that we reached the null entry. Then, I return this value instead of bool, and convey parsing failure separately.

To solve the first problem (and convey parsing failure) these functions now return either llvm::Error or llvm::Expected<DWARFEnumState>. Having this extra bit of information allows us to properly convey all 3 of us "error, bail out", "success, call this function again", and "success, don't call this function again".

In subsequent patches I plan to extend this pattern to the rest of the parsing interfaces, which will ultimately get all of the log statements and error reporting out of the low level parsing code and into the high level parsing code (e.g. SymbolFileDWARF, DWARFASTParserClang, etc).

Eventually, these same changes will have to be backported to LLVM's DWARF parser, but I feel like diverging in the short term is the quickest way to converge in the long term.

Diff Detail


Event Timeline

zturner created this revision.Mar 14 2019, 9:26 AM
zturner edited the summary of this revision. (Show Details)Mar 14 2019, 9:27 AM
aprantl added inline comments.Mar 14 2019, 9:42 AM
50 ↗(On Diff #190647)

half-finished comment?

zturner updated this revision to Diff 190654.Mar 14 2019, 9:55 AM

Fix half-written comment and add comments to other extraction function declarations.

clayborg accepted this revision.Mar 14 2019, 11:48 AM

As long as there is not a large performance regress when parsing large DWARF files this looks good to me. Abbreviation declarations are small, so I don't expect one.

37–42 ↗(On Diff #190654)

Seems like this would be malformed and should be an error. Something about NULL tag when valid tag was expected

This revision is now accepted and ready to land.Mar 14 2019, 11:48 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 12:05 PM