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

[George Rimar via Phabricator via llvm-commits]

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.

Or even better may be return not a bool, but enum, like PRINT_AND_STOP, JUST_PRINT, JUST_STOP, IGNORE_AND_CONTINUE. 
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.

> 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