[PATCH] D34328: [ELF] - Add ability for DWARFContextInMemory to exit early when any error happen.

[David Blaikie via llvm-commits]

On Sun, Jun 18, 2017 at 10:30 AM George Rimar via Phabricator <
reviews at reviews.llvm.org> wrote:

> grimar added a comment.
>
> In https://reviews.llvm.org/D34328#783474, @dblaikie wrote:
>
> > I'd probably favor the API change of returning
> Expected<unique_ptr<DWARFContext>> - potentially with a
> backwards-compatible API that maintains the old API (& calls the new API,
> reports any Error and silently returns the DWARFContext). Though as you
> say, some callers want to continue on error - so this new API wouldn't
> express that.
>
>
> Yes and I not sure how much is good to provide 2 API that are doing the
> same. I think doing something universal is cleaner,
> even if it will require more reasonable amount of changes around, probably.
>
> > Probably what might work better is if a callback for error handling was
> provided (a default one would be to maintain the existing behavior: print
> and continue. But other callers could say "print and stop". (eg: instead of
> "AllowsError", have llvm::function_ref<bool(Error E)> HandleError then do
> 'if (HandleError(std::move(E))) { return; }" or the like)
>
> That sounds really good for me. That way if we still have HadError flag or
> alike, we can print all errors in a format that is specific for any tool on
> its side and then after object is parsed check the flag and report final
> error then, if it is required.
>

The DWARFContext wouldn't have to have/maintain that flag though - a user
could stash that data when the callback is invoked.


> Or even better may be return not a bool, but enum, like PRINT_AND_STOP,
> JUST_PRINT, JUST_STOP, IGNORE_AND_CONTINUE.
>

Nah, probably not that way - one of the advantages of the callback API is
that it also removes the printing/reporting from libDebugInfo some more,
which is good. Makes it easier to test and use in different places (eg: in
a GUI app, or writing error messages in a different log format, etc)


> Last one may be used to emit warnings untill we do not implement some
> relocations for example, like for .gdb_index case (while it seems it does
> not affect index),
> and after that can switch to errors in one single change.
>
> One more case where it is useful is when we have callback on LLD side that
> print errors, we can implement the same error reporting strategy we already
> use now: a limit of 20 errors, we can print them on our size and return
> IGNORE_AND_CONTINUE,
> all time until reaching the limit - we just return JUST_STOP. Then check
> the HadError flag and stop link.
>

Sure, but that should (I think) all be able to be handled in the callback,
without having libDebugInfo/DWARFContext know anything about it other than
"hey, here's an error, handle it and tell me if you want me to keep going
or stop"


>
> > Why is the speed of lld important when the input is broken anyway?
> Printing all the errors may make it easier for a user to address them all
> without going through several slow link steps to get more errors (the same
> reason the compiler doesn't stop after the first error)? (eg: one corrupted
> compressed section, then one non-corrupt compressed section)
>
> it makes sence to me. Main reason for this patch is to add way to fail
> link somehow still, if we can report more errors, we can do that.
>
> > Also important to include test coverage - potentially in a unit test
> (though testing the IO would be tricky - but you could test if, given an
> input that had one broken, followed by a non-broken thing, then with the
> "stop on error" case returns no things and the "continue on error" returns
> the one non-broken thing.
>
> OK.
>
> Thanks a lot for your ideas, I will think a bit more on above and update
> patch with something different.
>
>
> https://reviews.llvm.org/D34328
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170618/e7bf8888/attachment.html>