Page MenuHomePhabricator

[Debuginfo][NFC] Unify error reporting routines inside DebugInfoDWARF.
Needs ReviewPublic

Authored by avl on Mon, Feb 10, 3:48 AM.

Details

Summary

Error reporting in DebugInfoDWARF library currently done in three ways :

  1. Direct calls to WithColor::error()/WithColor::warning()
  2. ErrorPolicy defaultErrorHandler(Error E);
  3. void dumpWarning(Error Warning);

additionally, other locations could have more variations:

lld/ELF/SyntheticSection.cpp

if (Error e = cu->tryExtractDIEsIfNeeded(false)) {
  error(toString(sec) + ": " + toString(std::move(e)));

DebugInfo/DWARF/DWARFUnit.cpp

if (Error e = tryExtractDIEsIfNeeded(CUDieOnly))
  WithColor::error() << toString(std::move(e));

Thus error reporting could look inconsistent. To have a consistent error
messages it is necessary to have a possibility to redefine error
reporting functions. This patch creates two handlers and allows to
redefine them. It also patches all places inside DebugInfoDWARF
to use these handlers.

The intention is always to use following handlers for error reporting
purposes inside DebugInfoDWARF:

DebugInfo/DWARF/DWARFContext.h

std::function<void(Error E)> RecoverableErrorHandler = [](Error E) {

defaultErrorHandler(std::move(E));

};
std::function<void(Error E)> WarningHandler = defaultWarningHandler;

Diff Detail

Event Timeline

avl created this revision.Mon, Feb 10, 3:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
avl added a comment.Mon, Feb 10, 4:17 AM

Unit tests pass. 62578 tests passed, 0 failed and 842 were skipped.

clang-tidy fail. clang-tidy found 0 errors and 1 warnings.

clang-format pass.

Overall, I like the principle of this change, but you should definitely hear what others have to say too.

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

Having a setter and getter for these seems a bit verbose, especially when you've got cases where you use the getter and then immediately call the return. Is there any particular reason to hide the handlers behind the interface?

llvm/lib/DWARFLinker/DWARFLinker.cpp
362 ↗(On Diff #243503)

I know it's not related to your change, but since you are touching the line anyway, could you change 8 /* Indent */ to /*Indent=*/8, please, as clang-format plays well with that.

400 ↗(On Diff #243503)

Ditto

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
642 ↗(On Diff #243503)

Have you considered using DWARFContext::defaultErrorHandler here?

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
240 ↗(On Diff #243503)

Same comment as elsewhere re. "indent"

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
162 ↗(On Diff #243503)

Same comments re. using DWARFContext::defaultErrorHandler and "Indent" comment.

avl marked 3 inline comments as done.Mon, Feb 10, 6:56 AM
avl added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

There is no particular reason. Having getter/setter allows to add some code like checking for nullptr, or dumping, or something else... If it looks verbose I would remove it.

llvm/lib/DWARFLinker/DWARFLinker.cpp
362 ↗(On Diff #243503)

Ok.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
642 ↗(On Diff #243503)

Ah, indeed. Would use it.

Overall it's a worthwhile direction - but please split this into smaller patches. General renamings separate, adding the functionality and using it in one place (preferably one that doesn't require function signature changes - one that already has the DWARFContext available, etc) & then a separate patch probably for each different error handling location that requires a function signature change. I think it'll be easier to review that way & make it clearer what changes were needed and how the error handling is being layered, etc.

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
94–96

Does this need to be wrapped in a lambda here?

358

@jhenderson : perhaps/did you mean "Is there any particular reason /not/ to hide the handlers behind the interface" - ie: I think you might've been suggesting that, rather than having "getRecoverableErrorHandler" the interface would have "emitRecoverableError(ERror E)" that calls the handler. Not exposing the handler itself to users of DWARFContext? Is that what you were/are suggesting? It sounds like a reasonable suggestion to me - at least worth asking/considering.

As for the setters - they seem to be uncalled right now? (just searching for them in this phab page doesn't show up any other instance of their names) - so probably best to leave them out until they're needed & we can discuss the merits of their use then?

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
307 ↗(On Diff #243503)

Please separate this sort of renaming into a pre or post patch to make this review easier/more targeted.

jhenderson added inline comments.Tue, Feb 11, 12:59 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

I don't think it's what I meant when I wrote that comment, but I think your suggestion manifesting itself as a half-formed thought was what triggered me to write a comment at all! I agree that an "emitRecovarableError(Error E)" signature would make a lot of sense - all it would do would be to call the handler with the passed-in error.

avl marked an inline comment as done.Tue, Feb 11, 2:00 AM
avl added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

I did not clearly get idea of "emitRecoverableError(ERror E)". My understanting is that it allows to change this usage :

public:  std::function<void(Error E)> RecoverableErrorHandler; // field of DWARFContext
...
DWARFContext.RecoverableErrorHandler(E);  // emit error.

with this usage:

DWARFContext.emitRecoverableError(E);  // emit error, RecoverableErrorHandler is not seen

But what about this usage :

func (std::function<void(Error E)> ErrorHandler)  // some function
      { ErrorHandler(Error); }


func(DWARFContext.RecoverableErrorHandler);  // pass handler to function

How would this case be resolved with emitRecoverableError() ?

Something like this:

func([&](Error E ) { DWARFContext.emitRecoverableError(std::move(E)); }   // pass handler to function

?

jhenderson added inline comments.Tue, Feb 11, 2:38 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

I'm not sure I follow your comments I'm afraid. The suggestion is to replace getRecoverableErrorHandler() with the following function:

void emitRecoverableError(Error Err) {
  RecoverableErrorHandler(std::move(Err));
}

Usage would be either:

Context.emitRecoverableError(createStringError("some text"));

or

doSomething(/*Callback=*/Context.emitRecoverableError);
...
void doSomething(function_ref<void(Error E)> Handler) {
...
  Handler(createStringError("some error"));
}

depending on whether the handler needs to be passed into the function, or the Context can be accessed directly. Effectively, in the second case, the real error handler is wrapped in another function that calls it.

avl marked an inline comment as done.Tue, Feb 11, 3:00 AM
avl added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

Ah, OK, Thanks. I misinterpreted second usage ...

avl marked an inline comment as done.Tue, Feb 11, 7:03 AM
avl added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

But... it looks like it does not work that way :

doSomething(/*Callback=*/Context.emitRecoverableError);

error: reference to non-static member function must be called

It needs to be

doSomething(/*Callback=*/[&](Error Err){Context.emitRecoverableError(std::move(Err))});

avl marked an inline comment as done.Tue, Feb 11, 1:51 PM
avl added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

@dblaikie David, How emitRecoverableError() should be used for this scenario ? :

doSomething(/*Callback=*/Context.emitRecoverableError);
...
void doSomething(function_ref<void(Error E)> Handler) {
...
  Handler(createStringError("some error"));
}

Exactly this variant seems not working, since std::function/function_ref could not be assigned to member function. it is necessary to use proxy lambda function or use std::bind to make it working.

But that probably is not very convenient solution.

dblaikie added inline comments.Tue, Feb 11, 2:03 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

Yeah, fair enough - seems like there are enough places that want to pass the handler along to that don't have a DWARFContext directly accessible (& makes sense to just pass down the handlers, rather than the whole DWARFContext). Seems fine as-is (as an accessor for the handler), then. Maybe @jhenderson has other thoughts since it was his feedback originally.

Still seems like the setters should be removed if (as they appear to be) they are unused.

avl marked an inline comment as done.Tue, Feb 11, 2:23 PM
avl added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

Ok. I also think it would be fine to use an accessor here :

std::function<void(Error Err)> getRecoverableErrorHandler {
    return RecoverableErrorHandler;
}

Though @jhenderson asked for publicly visible member originally:

std::function<void(Error Err)> RecoverableErrorHandler;

will wait for opinions more...

jhenderson added inline comments.Wed, Feb 12, 1:33 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
358

Public accessor is fine. I don't have a strong opinion on it either way.

avl updated this revision to Diff 244931.Mon, Feb 17, 2:15 AM

As it was requested, D74481, D74635 patches already integrate
most of functionality. This final part adds the possibility
to redefine error handlers into constructor of DWARFContext
and DWARFContext::create() functions.

jhenderson added inline comments.Tue, Feb 18, 3:02 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
115

This can just be RecoverablErrorHandler. No need for the The - constructor arguments can be named the same as the thing they're initialising. Same goes throughout most of the other functions too.

362

objectFileErrorsHandler -> objectFileErrorHandler

366–373

I don't think you need to pass this in as well as a RecoverableErrorHandler. The two shouldn't need to diverge, I think. Rather, have ObjectFileErrorHandler be defined in terms of RecoverableErrorHandler, as suggested below.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
1432

Shouldn't this function use the provided error handler rather than the default one?

In fact, maybe this should just be an inline lambda now.

avl marked 4 inline comments as done.Tue, Feb 18, 8:13 AM
avl added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
115

Ok.

362

ok.

366–373

There are two problems here:

  1. Using RecoverableErrorHandler for implementing ObjectFileErrorHandler.

    ObjectFileErrorHandler is a static function member, thus it could not be implemented using non-static function member RecoverableErrorHandler.
  1. Passing one Error handler instead of two error Handlers. i.e.:

    instead of passing objectFileErrorsHandler and RecoverableErrorHandler into DWARFContext::create() pass only one handler.

    As far as I understand original logic for DWARFContext::create(), there should be possible to pass handler which would stop processing if error occurred. i.e. it would return ErrorPolicy::Halt. That behavior is important for the creation function. But for the other DWARFContext errors that policy is not supported. It could probably be incorrect to use handler which returns ErrorPolicy::Halt for other DWARFContext errors.

    Since error logic for create() method and for other places differs - it looks correct to use different handlers. What do you think?
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
1432

since objectFileErrorsHandler is a static function - it could not use provided error handler(which is not-static member - RecoverableErrorHandler)

avl marked an inline comment as done.Thu, Feb 20, 5:14 AM
avl added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
366–373

@jhenderson James, what about passing two error handlers to "create" function?:

create(const object::ObjectFile &Obj, const LoadedObjectInfo *L = nullptr,
         function_ref<ErrorPolicy(Error)> ObjectFileErrorsHandler =
             objectFileErrorsHandler,
         std::string DWPName = "",
         std::function<void(Error)> RecoverableErrorHandler =
             WithColor::defaultErrorHandler,
         std::function<void(Error)> WarningHandler =
             WithColor::defaultWarningHandler);

Would it be OK ?

As an alternative objectFileErrorsHandler default implementation could be removed at all, with the price that every usage place would need to create it by itself:

create(const object::ObjectFile &Obj, const LoadedObjectInfo *L = nullptr,
         function_ref<ErrorPolicy(Error)> ObjectFileErrorsHandler =
             [](Error E)->ErrorPolicy {
               WithColor::defaultErrorHandler(std::move(E));
               return ErrorPolicy::Continue;
             },
         std::string DWPName = "",
         std::function<void(Error)> RecoverableErrorHandler =
             WithColor::defaultErrorHandler,
         std::function<void(Error)> WarningHandler =
             WithColor::defaultWarningHandler);
Context = DWARFContext::create(*Objects.second, nullptr,
                               [](Error E)->ErrorPolicy {
                                 WithColor::defaultErrorHandler(std::move(E));
                                 return ErrorPolicy::Continue;
                               }, Opts.DWPName);
avl updated this revision to Diff 245965.Fri, Feb 21, 12:39 PM

renamed parameters. objectFileErrorHandler is not addressed yet.