This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Jun 18 2017, 8:54 AM.

Details

Summary

Change introduces error reporting policy for DWARFContextInMemory.
New callback provided by client is able to handle error on it's
side and return Halt or Continue.

That allows to either keep current behavior when parser prints all errors
but continues parsing object or implement something very different, like
stop parsing on a first error and report an error from client side.

Diff Detail

Event Timeline

dblaikie edited edge metadata.Jun 18 2017, 9:29 AM

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.

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)

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)

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.

grimar added a comment.EditedJun 18 2017, 10:30 AM

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 side 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.

grimar planned changes to this revision.Jun 19 2017, 7:15 AM
grimar updated this revision to Diff 103956.Jun 26 2017, 8:13 AM
  • Addressed review comments.
  • Added testcase.
grimar updated this revision to Diff 103957.Jun 26 2017, 8:18 AM
  • Fix comment formatting.
dblaikie added inline comments.Jun 26 2017, 9:50 PM
include/llvm/DebugInfo/DWARF/DWARFContext.h
357

Might be worth introducing an enum ("Halt/Continue") rather than using a bool, so it's more obvious/legible which result means what.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
2150–2155

I thought one of the ideas of the yaml2obj stuff was to enable the writing of invalid inputs, etc - is that not the case? Should that functionality be added to the yaml2obj somehow rather than having big byte arrays?

grimar updated this revision to Diff 104153.Jun 27 2017, 6:58 AM
  • Addressed review comments.
include/llvm/DebugInfo/DWARF/DWARFContext.h
357

Done.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
2150–2155

What I was trying to achieve here is to produce broken object::ObjectFile somehow to be able to test first constructor of DWARFContextInMemory with new functionality.
Extending yaml2obj to produce StringRef (if I correctly understood the suggestion) would require doing some librarification for it. Like splitting part of yaml2obj into separate library probably. And then test would be able to use such library.

It probably can be usefull (I am not sure how much other users we may have, looks can be usefull for testcases like this), but sounds as a large enough separate change. It is unclear for me if introducing a new library worth that atm.

Fortunately I found alternative way to achieve what I wanted initially in this testcase, without using additional arrays and dependency on yaml2obj. Solution partially consistent with other testcases in this file which aslo use dwarfgen::Generator but for different things.

Alternative solution I was thinking about - is to introduce --error-limit=X option to llvm-dwarfdump. It probably may be usefull itself and would allow to write testcase in a regular manner (separate test file + one/two broken input files and regular tools calls).

dblaikie accepted this revision.Jun 27 2017, 7:58 AM

Looks great - thanks!

This revision is now accepted and ready to land.Jun 27 2017, 7:58 AM

Looks great - thanks!

Thanks for review ! I'll commit this at my tomorrow morning.
(want to be online for case if this breaks any slow BB or something else).

grimar edited the summary of this revision. (Show Details)Jun 27 2017, 11:55 PM
This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.
llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
18 ↗(On Diff #104353)

s/Codegen/CodeGen/

grimar added inline comments.Jun 28 2017, 12:40 AM
llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
18 ↗(On Diff #104353)

Ah, thanks !