[PATCH] D44382: [DWARF] Rework debug line parsing to use llvm::Error

[Rafael Avila de Espindola via llvm-commits]

James Henderson via Phabricator via llvm-commits
<llvm-commits at lists.llvm.org> writes:

> jhenderson created this revision.
> jhenderson added reviewers: probinson, dblaikie, JDevlieghere, aprantl, labath.
>
> The .debug_line parser previously reported errors by printing to stderr and return false. This is not particularly helpful for clients of the library code, as it prevents them from handling the errors in a manner based on the calling context. This change switches to using llvm::Error to indicate what problems were detected during parsing, and has updated clients to handle the errors in a location-specific manner. In general, this means that they continue to do the same thing to external users. Below, I have outlined what the known behaviour changes are, relating to this change.
>
> There are three levels of "errors" in the new error mechanism, to broadly distinguish between different fail states of the parser, since not every failure will prevent parsing of the unit, or of subsequent unit. Fatal errors represent errors that prevent reading the unit length field. If this happens, it will be impossible to know where the next unit starts, so this will prevent further parsing of both the current unit and any subsequent ones. Major errors represent errors that prevent reading the remainder of the table, e.g. because 
> the version is unsupported. Minor errors represent problems with parsing that do not prevent attempting to continue reading the table. The only example of this currently is when the last sequence of a unit is unterminated. However, I think it would be good to change the handling of unrecognised opcodes to report as minor errors as well, rather than just printing to the stream if --verbose is used (this would be a subsequent change however).

The main advantaged of using Expected is that it is not possible to
access an invalid object.

If possible I think it would be better to reserve Error/Expected for "it
was not possible to produce the requested result" and have some
logging/diagnostics/callback system for things that can be ignored.

For example, getOrParseLineTable could return Expected<const LineTable
*> and take a callback for non fatal issues:

if (Error = Callback(DebugLineDiag::Minor,
                                    "last sequence in debug line table is not"
                                    " terminated!"))
    return Error;

And it is up to the callback whether it wants getOrParseLineTable to fail.

Alternatively it could return Expected<std::pair<const LineTable *, Warnings>>.

Cheers,
Rafael