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

[rupprecht]

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.

[alexander-shaposhnikov]

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() ?

[rupprecht]

• Add comment about using createFileError

[rupprecht]

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.

[jhenderson]

LGTM.

llvm/test/tools/llvm-objcopy/ELF/fail-no-output-directory.test

 # RUN: yaml2obj %s > %t
 # RUN: not llvm-objcopy %t no/such/dir 2>&1 | FileCheck %s
-# CHECK: failed to open no/such/dir:
+# CHECK: error: 'no/such/dir': No such file or directory
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 

llvm/tools/llvm-objcopy/Buffer.h

 // abstract interface and doesn't depend on a particular implementation.
 // TODO: refactor the buffer classes in LLVM to enable us to use them here
 // directly.
 class Buffer {
   StringRef Name;
 
 public:
   virtual ~Buffer();
-  virtual void allocate(size_t Size) = 0;
+  virtual Error allocate(size_t Size) = 0;
   virtual uint8_t *getBufferStart() = 0;
   virtual Error commit() = 0;
 
   explicit Buffer(StringRef Name) : Name(Name) {}
   StringRef getName() const { return Name; }
 };
 
 class FileBuffer : public Buffer {
   std::unique_ptr<FileOutputBuffer> Buf;
 
 public:
-  void allocate(size_t Size) override;
+  Error allocate(size_t Size) override;
   uint8_t *getBufferStart() override;
   Error commit() override;
 
   explicit FileBuffer(StringRef FileName) : Buffer(FileName) {}
 };
 
 class MemBuffer : public Buffer {
   std::unique_ptr<WritableMemoryBuffer> Buf;
 
 public:
-  void allocate(size_t Size) override;
+  Error allocate(size_t Size) override;
   uint8_t *getBufferStart() override;
   Error commit() override;
 
   explicit MemBuffer(StringRef Name) : Buffer(Name) {}
 
   std::unique_ptr<WritableMemoryBuffer> releaseMemoryBuffer();
 };
 
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_BUFFER_H

llvm/tools/llvm-objcopy/Buffer.cpp

 #include "llvm/Support/MemoryBuffer.h"
 #include <memory>
 
 namespace llvm {
 namespace objcopy {
 
 Buffer::~Buffer() {}
 
-void FileBuffer::allocate(size_t Size) {
+Error FileBuffer::allocate(size_t Size) {
   Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
       FileOutputBuffer::create(getName(), Size, FileOutputBuffer::F_executable);
-  handleAllErrors(BufferOrErr.takeError(), [this](const ErrorInfoBase &E) {
-    error("failed to open " + getName() + ": " + E.message());
-  });
+  // FileOutputBuffer::create() returns an Error that is just a wrapper around
+  // std::error_code. Wrap it in FileError to include the actual filename.
+  if (!BufferOrErr)
+    return createFileError(getName(), BufferOrErr.takeError());
   Buf = std::move(*BufferOrErr);
+  return Error::success();
 }
 
-Error FileBuffer::commit() { return Buf->commit(); }
+Error FileBuffer::commit() {
+  Error Err = Buf->commit();
+  // FileOutputBuffer::commit() returns an Error that is just a wrapper around
+  // std::error_code. Wrap it in FileError to include the actual filename.
+  return Err ? createFileError(getName(), std::move(Err)) : std::move(Err);
+}
 
 uint8_t *FileBuffer::getBufferStart() {
   return reinterpret_cast<uint8_t *>(Buf->getBufferStart());
 }
 
-void MemBuffer::allocate(size_t Size) {
+Error MemBuffer::allocate(size_t Size) {
   Buf = WritableMemoryBuffer::getNewMemBuffer(Size, getName());
+  return Error::success();
 }
 
 Error MemBuffer::commit() { return Error::success(); }
 
 uint8_t *MemBuffer::getBufferStart() {
   return reinterpret_cast<uint8_t *>(Buf->getBufferStart());
 }
 
 std::unique_ptr<WritableMemoryBuffer> MemBuffer::releaseMemoryBuffer() {
   return std::move(Buf);
 }
 
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/COFF/Writer.cpp

if (StrTabBuilder.getSize() > 4 || !Obj.IsPE) {
     Ptr += StrTabBuilder.getSize();
   }
 }
 
 Error COFFWriter::write(bool IsBigObj) {
   if (Error E = finalize(IsBigObj))
     return E;
 
-  Buf.allocate(FileSize);
+  if (Error E = Buf.allocate(FileSize))
+    return E;
 
   writeHeaders(IsBigObj);
   writeSections();
   if (IsBigObj)
     writeSymbolStringTables<coff_symbol32>();
   else
     writeSymbolStringTables<coff_symbol16>();
 

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

static void splitDWOToFile(const CopyConfig &Config, const Reader &Reader,
                            StringRef File, ElfType OutputElfType) {
   auto DWOFile = Reader.create();
   DWOFile->removeSections(
       [&](const SectionBase &Sec) { return onlyKeepDWOPred(*DWOFile, Sec); });
   if (Config.OutputArch)
     DWOFile->Machine = Config.OutputArch.getValue().EMachine;
   FileBuffer FB(File);
   auto Writer = createWriter(Config, *DWOFile, FB, OutputElfType);
-  Writer->finalize();
-  Writer->write();
+  if (Error E = Writer->finalize())
+    error(std::move(E));
+  if (Error E = Writer->write())
+    error(std::move(E));
 }
 
 static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
                                Object &Obj) {
   for (auto &Sec : Obj.sections()) {
     if (Sec.Name == SecName) {
       if (Sec.OriginalData.empty())
         return make_error<StringError>("Can't dump section \"" + SecName +
void executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
 
   // Prefer OutputArch (-O<format>) if set, otherwise fallback to BinaryArch
   // (-B<arch>).
   const ElfType OutputElfType = getOutputElfType(
       Config.OutputArch ? Config.OutputArch.getValue() : Config.BinaryArch);
   handleArgs(Config, *Obj, Reader, OutputElfType);
   std::unique_ptr<Writer> Writer =
       createWriter(Config, *Obj, Out, OutputElfType);
-  Writer->finalize();
-  Writer->write();
+  if (Error E = Writer->finalize())
+    error(std::move(E));
+  if (Error E = Writer->write())
+    error(std::move(E));
 }
 
 void executeObjcopyOnBinary(const CopyConfig &Config,
                             object::ELFObjectFileBase &In, Buffer &Out) {
   ELFReader Reader(&In);
   std::unique_ptr<Object> Obj = Reader.create();
   // Prefer OutputArch (-O<format>) if set, otherwise infer it from the input.
   const ElfType OutputElfType =
void executeObjcopyOnBinary(const CopyConfig &Config,
 
   if (!Config.BuildIdLinkDir.empty() && Config.BuildIdLinkInput) {
     linkToBuildIdDir(Config, Config.InputFilename,
                      Config.BuildIdLinkInput.getValue(), BuildIdBytes);
   }
   handleArgs(Config, *Obj, Reader, OutputElfType);
   std::unique_ptr<Writer> Writer =
       createWriter(Config, *Obj, Out, OutputElfType);
-  Writer->finalize();
-  Writer->write();
+  if (Error E = Writer->finalize())
+    error(std::move(E));
+  if (Error E = Writer->write())
+    error(std::move(E));
   if (!Config.BuildIdLinkDir.empty() && Config.BuildIdLinkOutput) {
     linkToBuildIdDir(Config, Config.OutputFilename,
                      Config.BuildIdLinkOutput.getValue(), BuildIdBytes);
   }
 }
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/ELF/Object.h

 
 class Writer {
 protected:
   Object &Obj;
   Buffer &Buf;
 
 public:
   virtual ~Writer();
-  virtual void finalize() = 0;
-  virtual void write() = 0;
+  virtual Error finalize() = 0;
+  virtual Error write() = 0;
 
   Writer(Object &O, Buffer &B) : Obj(O), Buf(B) {}
 };
 
 template <class ELFT> class ELFWriter : public Writer {
 private:
   using Elf_Addr = typename ELFT::Addr;
   using Elf_Shdr = typename ELFT::Shdr;
private:
   std::unique_ptr<ELFSectionWriter<ELFT>> SecWriter;
 
   size_t totalSize() const;
 
 public:
   virtual ~ELFWriter() {}
   bool WriteSectionHeaders = true;
 
-  void finalize() override;
-  void write() override;
+  Error finalize() override;
+  Error write() override;
   ELFWriter(Object &Obj, Buffer &Buf, bool WSH)
       : Writer(Obj, Buf), WriteSectionHeaders(WSH) {}
 };
 
 class BinaryWriter : public Writer {
 private:
   std::unique_ptr<BinarySectionWriter> SecWriter;
 
   uint64_t TotalSize;
 
 public:
   ~BinaryWriter() {}
-  void finalize() override;
-  void write() override;
+  Error finalize() override;
+  Error write() override;
   BinaryWriter(Object &Obj, Buffer &Buf) : Writer(Obj, Buf) {}
 };
 
 class SectionBase {
 public:
   std::string Name;
   Segment *ParentSegment = nullptr;
   uint64_t HeaderOffset;

llvm/tools/llvm-objcopy/ELF/Object.cpp

 template <class ELFT> size_t ELFWriter<ELFT>::totalSize() const {
   // We already have the section header offset so we can calculate the total
   // size by just adding up the size of each section header.
   auto NullSectionSize = WriteSectionHeaders ? sizeof(Elf_Shdr) : 0;
   return Obj.SHOffset + size(Obj.sections()) * sizeof(Elf_Shdr) +
          NullSectionSize;
 }
 
-template <class ELFT> void ELFWriter<ELFT>::write() {
+template <class ELFT> Error ELFWriter<ELFT>::write() {
   writeEhdr();
   writePhdrs();
   writeSectionData();
   if (WriteSectionHeaders)
     writeShdrs();
-  if (auto E = Buf.commit())
-    reportError(Buf.getName(), errorToErrorCode(std::move(E)));
+  return Buf.commit();
 }
 
-template <class ELFT> void ELFWriter<ELFT>::finalize() {
+template <class ELFT> Error ELFWriter<ELFT>::finalize() {
   // It could happen that SectionNames has been removed and yet the user wants
   // a section header table output. We need to throw an error if a user tries
   // to do that.
   if (Obj.SectionNames == nullptr && WriteSectionHeaders)
     error("Cannot write section header table because section header string "
           "table was removed.");
 
   Obj.sortSections();
template <class ELFT> Error ELFWriter<ELFT>::finalize() {
   for (auto &Section : Obj.sections()) {
     Section.HeaderOffset = Offset;
     Offset += sizeof(Elf_Shdr);
     if (WriteSectionHeaders)
       Section.NameIndex = Obj.SectionNames->findIndex(Section.Name);
     Section.finalize();
   }
 
-  Buf.allocate(totalSize());
+  if (Error E = Buf.allocate(totalSize()))
+    return E;
   SecWriter = llvm::make_unique<ELFSectionWriter<ELFT>>(Buf);
+  return Error::success();
 }
 
-void BinaryWriter::write() {
+Error BinaryWriter::write() {
   for (auto &Section : Obj.sections()) {
     if ((Section.Flags & SHF_ALLOC) == 0)
       continue;
     Section.accept(*SecWriter);
   }
-  if (auto E = Buf.commit())
-    reportError(Buf.getName(), errorToErrorCode(std::move(E)));
+  return Buf.commit();
 }
 
-void BinaryWriter::finalize() {
+Error BinaryWriter::finalize() {
   // TODO: Create a filter range to construct OrderedSegments from so that this
   // code can be deduped with assignOffsets above. This should also solve the
   // todo below for LayoutSections.
   // We need a temporary list of segments that has a special order to it
   // so that we know that anytime ->ParentSegment is set that segment has
   // already had it's offset properly set. We only want to consider the segments
   // that will affect layout of allocated sections so we only add those.
   std::vector<Segment *> OrderedSegments;
Error BinaryWriter::finalize() {
   // LayoutSections, because we want to truncate the last segment to the end of
   // its last section, to match GNU objcopy's behaviour.
   TotalSize = 0;
   for (const auto &Section : AllocatedSections) {
     if (Section->Type != SHT_NOBITS)
       TotalSize = std::max(TotalSize, Section->Offset + Section->Size);
   }
 
-  Buf.allocate(TotalSize);
+  if (Error E = Buf.allocate(TotalSize))
+    return E;
   SecWriter = llvm::make_unique<BinarySectionWriter>(Buf);
+  return Error::success();
 }
 
 template class ELFBuilder<ELF64LE>;
 template class ELFBuilder<ELF64BE>;
 template class ELFBuilder<ELF32LE>;
 template class ELFBuilder<ELF32BE>;
 
 template class ELFWriter<ELF64LE>;
 template class ELFWriter<ELF64BE>;
 template class ELFWriter<ELF32LE>;
 template class ELFWriter<ELF32BE>;
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/llvm-objcopy.h

 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
 #include <string>
 
 namespace llvm {
 namespace objcopy {
 
 LLVM_ATTRIBUTE_NORETURN extern void error(Twine Message);
+LLVM_ATTRIBUTE_NORETURN extern void error(Error E);
 LLVM_ATTRIBUTE_NORETURN extern void reportError(StringRef File, Error E);
 LLVM_ATTRIBUTE_NORETURN extern void reportError(StringRef File,
                                                 std::error_code EC);
 
 // This is taken from llvm-readobj.
 // [see here](llvm/tools/llvm-readobj/llvm-readobj.h:38)
 template <class T> T unwrapOrError(Expected<T> EO) {
   if (EO)

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

 StringRef ToolName;
 
 LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
   WithColor::error(errs(), ToolName) << Message << ".\n";
   errs().flush();
   exit(1);
 }
 
+LLVM_ATTRIBUTE_NORETURN void error(Error E) {
+  assert(E);
+  std::string Buf;
+  raw_string_ostream OS(Buf);
+  logAllUnhandledErrors(std::move(E), OS);
+  OS.flush();
+  WithColor::error(errs(), ToolName) << Buf;
+  exit(1);
+}
+
 LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, std::error_code EC) {
   assert(EC);
   WithColor::error(errs(), ToolName)
       << "'" << File << "': " << EC.message() << ".\n";
   exit(1);
 }
 
 LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, Error E) {
for (const NewArchiveMember &Member : NewMembers) {
     // FileOutputBuffer::create, for regular files (that is the case for
     // deepWriteArchive) FileOutputBuffer::create will return OnDiskBuffer.
     // OnDiskBuffer uses a temporary file and then renames it. So in reality
     // there is no inefficiency / duplicated in-memory buffers in this case. For
     // now in-memory buffers can not be completely avoided since
     // NewArchiveMember still requires them even though writeArchive does not
     // write them on disk.
     FileBuffer FB(Member.MemberName);
-    FB.allocate(Member.Buf->getBufferSize());
+    if (Error E = FB.allocate(Member.Buf->getBufferSize()))
+      return E;
     std::copy(Member.Buf->getBufferStart(), Member.Buf->getBufferEnd(),
               FB.getBufferStart());
-    if (auto E = FB.commit())
+    if (Error E = FB.commit())
       return E;
   }
   return Error::success();
 }
 
 /// The function executeObjcopyOnRawBinary does the dispatch based on the format
 /// of the output specified by the command line options.
 static void executeObjcopyOnRawBinary(const CopyConfig &Config,