This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Return Error from Buffer::allocate(), [ELF]Writer::finalize(), and [ELF]Writer::commit()
ClosedPublic

Authored by rupprecht on Jan 18 2019, 12:57 PM.

Details

Summary

This patch changes a few methods to return Error instead of manually calling error/reportError to abort. This will make it easier to extract into a library.

Note that error() takes just a string (this patch also adds an overload that takes an Error), while reportError() takes string + [error/code]. To help unify things, use FileError to associate a given filename with an error. Note that this takes some special care (for now), e.g. calling reportError(FileName, <something that could be FileError>) will duplicate the filename. The goal is to eventually remove reportError() and have every error associated with a file to be a FileError, and just one error handling block at the tool level.

This change was suggested in D56806. I took it a little further than suggested, but completely fixing llvm-objcopy will take a couple more patches. If this approach looks good, I'll commit this and apply similar patche(s) for the rest.

This change is NFC in terms of non-error related code, although the error message changes in one context.

Event Timeline

rupprecht created this revision.Jan 18 2019, 12:57 PM
mstorsjo set the repository for this revision to rL LLVM.Jan 18 2019, 1:01 PM
mstorsjo added a subscriber: llvm-commits.

i have one inline comment, but otherwise this looks good to me

llvm/tools/llvm-objcopy/Buffer.cpp
25

just in case: why do we use createFileError here instead of propagating the original error BufferOrErr.takeError() ?

This revision is now accepted and ready to land.Jan 18 2019, 2:49 PM
rupprecht updated this revision to Diff 182621.Jan 18 2019, 3:43 PM
rupprecht marked an inline comment as done.
  • Add comment about using createFileError
rupprecht marked an inline comment as done.Jan 18 2019, 3:55 PM
rupprecht added inline comments.
llvm/tools/llvm-objcopy/Buffer.cpp
25

Left a comment inline explaining why.

On the base side, this was manually calling error(FileName + ": " + Message) to include the filename in the error, which is analagous to reportError(FileName, Error) used elsewhere in llvm-objcopy. If we want to bubble up Errors to the top level instead of calling error/reportError with the filename here, we need to wrap it in FileError.
I guess there might be reasons why FileOutputBuffer methods don't return FileErrors. It would be nice if this code didn't assume that FileError *isn't* being used, e.g.

// Wraps errors into FileErrors, but doesn't re-wrap FileErrors
Error maybeWrapFileError(StringRef FileName, Error Err) {
  return Err.isA<FileError>() ? std::move(Err) : createFileError(FileName, std::move(Err));
}

Depending on how common this is when migrating the rest of these, I might put this utility somewhere in libSupport. For now, it's just these two spots.

This revision was automatically updated to reflect the committed changes.