<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sun, Jun 18, 2017 at 10:30 AM George Rimar via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">grimar added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D34328#783474" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34328#783474</a>, @dblaikie wrote:<br>
<br>
> 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.<br>
<br>
<br>
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,<br>
even if it will require more reasonable amount of changes around, probably.<br>
<br>
> 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)<br>
<br>
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.<br></blockquote><div><br>The DWARFContext wouldn't have to have/maintain that flag though - a user could stash that data when the callback is invoked.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Or even better may be return not a bool, but enum, like PRINT_AND_STOP, JUST_PRINT, JUST_STOP, IGNORE_AND_CONTINUE.<br></blockquote><div><br>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)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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),<br>
and after that can switch to errors in one single change.<br>
<br>
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,<br>
all time until reaching the limit - we just return JUST_STOP. Then check the HadError flag and stop link.<br></blockquote><div><br>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"<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> 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)<br>
<br>
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.<br>
<br>
> 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.<br>
<br>
OK.<br>
<br>
Thanks a lot for your ideas, I will think a bit more on above and update patch with something different.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D34328" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34328</a><br>
<br>
<br>
<br>
</blockquote></div></div>