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

[David Blaikie via llvm-commits]

+Lang for thoughts of idiomatic/ideal Error usage.

I tend to agree with Rafael's thoughts here - a callback of some kind for
warnings seems best.

On Mon, Mar 12, 2018 at 12:59 PM Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180315/48e329de/attachment.html>