Page MenuHomePhabricator

Linker: error_code'ify the IR mover. NFC.
ClosedPublic

Authored by pcc on May 23 2016, 7:49 PM.

Details

Summary

This will be needed in order to consistently return an error_code
to clients of the API being developed in D20268.

Diff Detail

Event Timeline

pcc updated this revision to Diff 58182.May 23 2016, 7:49 PM
pcc retitled this revision from to Linker: error_code'ify the IR mover. NFC..
pcc updated this object.
pcc added a reviewer: rafael.
pcc added subscribers: llvm-commits, mehdi_amini.

You may want to use Error (include/llvm/Support/Error.h) rather than std::error_code, though it should be pretty easy to switch over after the fact too.

  • Lang.
rafael edited edge metadata.May 24 2016, 3:36 AM
rafael added a subscriber: rafael.

Let's please use Error from the start.

pcc added a comment.May 24 2016, 12:50 PM

Okay, I'll do that.

I was unaware of the intended migration to llvm::Error, but I probably could have been made aware if one of the std::error_category derived classes I cargo culted from had a relevant FIXME. I've prepared D20592 which adds FIXMEs to all derived classes.

Following on from the discussion on http://reviews.llvm.org/D20592 (I might write up a blog post on this soon, but this is the quick version):

While developing Error I tried transitioning MachOObjectFile to use the new type and quickly ran into a problem: The libObject API had a lot of clients, and those clients all used std::error_code. Trying to fix all the clients resulted in an impractically large patch, and I realized that other people making the switch would run into the same problem. For that reason I decided that during this transition period the best thing to do was require Error and error_code be convertible from one another (same with Expected and ErrorOr). This allows us to change APIs very surgically, right down to changes in individual variables / functions, by just wrapping the types with the conversion operators when used. That's great for incrementally moving over from std::error_code to Error, but leaves us with this wart: For now every Error needs to be convertible into an equivalent error_code. Often that means that the easiest way to transition an API to Error is to introduce a new std::error_category (as you've done), update the API to return Error, and then construct your error values using:

errorCodeToError(<insert error code here>);

After you've updated the API you can start selectively adding custom error classes and mapping them to errors in your category. Eventually, once everyone is moved over, we can remove the error category and just deal with the custom error type.

pcc updated this revision to Diff 58337.May 24 2016, 3:30 PM
pcc edited edge metadata.
  • Return an llvm::Error instead
pcc added a comment.May 24 2016, 3:30 PM

Thanks Lang. This new patch does as you suggested: it retains the std::error_category, but converts the error_code to an Error at the last moment. I have also updated the call sites to handle a non-null Error in the same way as they handled a true return value before.

Taking a look.

With this patch the linker would be using all 3 error handling systems
in llvm, which is way too much.

What I think needs to happen is for it to have an Error class that is
not converted to error_code. We also have to decide if we are keeping
a list of error or just failing early. I don't think it is worth it
keeping multiple errors, so I would suggest refactoring the code to
return early and only store the error when it hits an interface that
returns void.

We should also stop logging error to the streamer and instead have the
caller uses log on the Error.

The attached patch implements part of that. What do you think?

Cheers,
Rafael

pcc updated this revision to Diff 58703.May 26 2016, 3:30 PM

New patch

Mostly looks good, but StringError as-is would be dangerous as a general utility - conversion is impossible but we're unlikely to spot that during development because we're bad at testing error cases. Instead there'll be UB out in the wild when we hit it.

Let me fix up the indescribable error stuff that I've got and commit a safer version of StringError that uses it.

I've added StringError in r270948 (+r270950) with an extra argument to provide the error_code conversion.

You can replace uses in this patch with:

make_error<StringError>(<msg>, inconvertibleErrorCode());

It's a bit more verbose, but I want to make sure people explicitly opt in to inconvertibility. In your case you can probably just wrap it with a:

Error stringErr(const Twine &T);

convenience function.

pcc updated this revision to Diff 58744.May 26 2016, 7:37 PM
  • Use lhames's StringError
lhames accepted this revision.May 26 2016, 8:03 PM
lhames added a reviewer: lhames.

Otherwise LGTM.

lib/Linker/LinkModules.cpp
588 ↗(On Diff #58744)

Minor nitpick - You don't actually need to return Error::success() from handlers: the ErrorHandlerTraits will infer it if there's no return statement.

You could also put the call inline in the handleAllErrors call:

handleAllErrors(
  Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(),
             [this](GlobalValue &GV, IRMover::ValueAdder Add) {
               addLazyFor(GV, Add);
             }),
  [&](ErrorInfoBase &EIB) {
    DstM.getContext().diagnose(LinkDiagnosticInfo(DS_Error, EIB.message()));
    HasErrors = true;
  });

I haven't formed any strong feelings on Error style yet though. If you prefer to keep the existing style to show a clear separation between the call and the handlers that's fine by me.

tools/gold/gold-plugin.cpp
1156 ↗(On Diff #58744)

Likewise here.

This revision is now accepted and ready to land.May 26 2016, 8:03 PM
This revision was automatically updated to reflect the committed changes.