[llvm-objcopy][NFC] More error propagation

[rupprecht]

Summary

Do some more error cleanup, removing some dependencies from llvm-objcopy's error/reportError in [ELF/COFF]Objcopy methods.

[jakehehrlich]

OMG thank you. I've been wanting this to happen so badly. LGTM.

[jhenderson]

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

147–148 ↗

(On Diff #184199)

You seem to have missed several reportError calls in this function. Was that intentional?

[rupprecht]

In D57423#1376475, @jakehehrlich wrote:

OMG thank you. I've been wanting this to happen so badly. LGTM.

:)
There's about 40 calls to error/reportError from outside the main driver class, this fixes 10-ish of them, so still over half of them to go...

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

147–148 ↗

(On Diff #184199)

Nope -- missed this one, I'll follow up with a fix. (Accidentally checked in this one instead of D57248 before addressing this, sorry... was in the wrong git branch :( ).

llvm/trunk/test/tools/llvm-objcopy/ELF/bad-build-id.test

 # RUN: yaml2obj %s > %t
 # RUN: not llvm-objcopy --build-id-link-dir=%t-dir --build-id-link-input=.debug %t 2>&1 >/dev/null | FileCheck %s
 
-# CHECK: build ID in file {{.*}} is smaller than two bytes.
+# CHECK: build ID is smaller than two bytes.
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:

llvm/trunk/tools/llvm-objcopy/COFF/COFFObjcopy.h

 //===- COFFObjcopy.h --------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_OBJCOPY_COFFOBJCOPY_H
 #define LLVM_TOOLS_OBJCOPY_COFFOBJCOPY_H
 
 namespace llvm {
+class Error;
 
 namespace object {
 class COFFObjectFile;
 } // end namespace object
 
 namespace objcopy {
 struct CopyConfig;
 class Buffer;
 
 namespace coff {
-void executeObjcopyOnBinary(const CopyConfig &Config,
+Error executeObjcopyOnBinary(const CopyConfig &Config,
                              object::COFFObjectFile &In, Buffer &Out);
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_COFFOBJCOPY_H

llvm/trunk/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

if (!Config.BuildIdLinkDir.empty() || Config.BuildIdLinkInput ||
       Config.StripSections || Config.Weaken || Config.DecompressDebugSections) {
     return createStringError(llvm::errc::invalid_argument,
                              "Option not supported by llvm-objcopy for COFF");
   }
 
   return Error::success();
 }
 
-void executeObjcopyOnBinary(const CopyConfig &Config,
-                            COFFObjectFile &In, Buffer &Out) {
+Error executeObjcopyOnBinary(const CopyConfig &Config, COFFObjectFile &In,
+                             Buffer &Out) {
   COFFReader Reader(In);
   Expected<std::unique_ptr<Object>> ObjOrErr = Reader.create();
   if (!ObjOrErr)
-    reportError(Config.InputFilename, ObjOrErr.takeError());
+    return createFileError(Config.InputFilename, ObjOrErr.takeError());
   Object *Obj = ObjOrErr->get();
   assert(Obj && "Unable to deserialize COFF object");
   if (Error E = handleArgs(Config, *Obj))
-    reportError(Config.InputFilename, std::move(E));
+    return createFileError(Config.InputFilename, std::move(E));
   COFFWriter Writer(*Obj, Out);
   if (Error E = Writer.write())
-    reportError(Config.OutputFilename, std::move(E));
+    return createFileError(Config.OutputFilename, std::move(E));
+  return Error::success();
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.h

 //===- ELFObjcopy.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_OBJCOPY_ELFOBJCOPY_H
 #define LLVM_TOOLS_OBJCOPY_ELFOBJCOPY_H
 
 namespace llvm {
+class Error;
 class MemoryBuffer;
 
 namespace object {
 class ELFObjectFileBase;
 } // end namespace object
 
 namespace objcopy {
 struct CopyConfig;
 class Buffer;
 
 namespace elf {
-void executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
+Error executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
                                 Buffer &Out);
-void executeObjcopyOnBinary(const CopyConfig &Config,
+Error executeObjcopyOnBinary(const CopyConfig &Config,
                              object::ELFObjectFileBase &In, Buffer &Out);
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_ELFOBJCOPY_H

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

if (auto EC = sys::fs::create_hard_link(ToLink, Path)) {
     if (sys::fs::exists(Path))
       sys::fs::remove(Path);
     EC = sys::fs::create_hard_link(ToLink, Path);
     if (EC)
       error("cannot link " + ToLink + " to " + Path + ": " + EC.message());
   }
 }
 
-static void splitDWOToFile(const CopyConfig &Config, const Reader &Reader,
+static Error 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);
   if (Error E = Writer->finalize())
-    error(std::move(E));
-  if (Error E = Writer->write())
-    error(std::move(E));
+    return E;
+  return Writer->write();
 }
 
 static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
                                Object &Obj) {
   for (auto &Sec : Obj.sections()) {
     if (Sec.Name == SecName) {
       if (Sec.OriginalData.empty())
         return createStringError(

 
 // This function handles the high level operations of GNU objcopy including
 // handling command line options. It's important to outline certain properties
 // we expect to hold of the command line operations. Any operation that "keeps"
 // should keep regardless of a remove. Additionally any removal should respect
 // any previous removals. Lastly whether or not something is removed shouldn't
 // depend a) on the order the options occur in or b) on some opaque priority
 // system. The only priority is that keeps/copies overrule removes.
-static void handleArgs(const CopyConfig &Config, Object &Obj,
+static Error handleArgs(const CopyConfig &Config, Object &Obj,
                         const Reader &Reader, ElfType OutputElfType) {
 
-  if (!Config.SplitDWO.empty()) {
-    splitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType);
-  }
+  if (!Config.SplitDWO.empty())
+    if (Error E =
+            splitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType))
+      return E;
+
   if (Config.OutputArch)
     Obj.Machine = Config.OutputArch.getValue().EMachine;
 
   // TODO: update or remove symbols only if there is an option that affects
   // them.
   if (Obj.SymbolTable) {
     Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
       if (!Sym.isCommon() &&
static Error handleArgs(const CopyConfig &Config, Object &Obj,
   if (!Config.AddSection.empty()) {
     for (const auto &Flag : Config.AddSection) {
       std::pair<StringRef, StringRef> SecPair = Flag.split("=");
       StringRef SecName = SecPair.first;
       StringRef File = SecPair.second;
       ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
           MemoryBuffer::getFile(File);
       if (!BufOrErr)
-        reportError(File, BufOrErr.getError());
+        return createFileError(File, errorCodeToError(BufOrErr.getError()));
       std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
       ArrayRef<uint8_t> Data(
           reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
           Buf->getBufferSize());
       OwnedDataSection &NewSection =
           Obj.addSection<OwnedDataSection>(SecName, Data);
       if (SecName.startswith(".note") && SecName != ".note.GNU-stack")
         NewSection.Type = SHT_NOTE;
     }
   }
 
   if (!Config.DumpSection.empty()) {
     for (const auto &Flag : Config.DumpSection) {
       std::pair<StringRef, StringRef> SecPair = Flag.split("=");
       StringRef SecName = SecPair.first;
       StringRef File = SecPair.second;
       if (Error E = dumpSectionToFile(SecName, File, Obj))
-        reportError(Config.InputFilename, std::move(E));
+        return createFileError(Config.InputFilename, std::move(E));
     }
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
+
+  return Error::success();
 }
 
-void executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
+Error executeObjcopyOnRawBinary(const CopyConfig &Config, MemoryBuffer &In,
                                 Buffer &Out) {
   BinaryReader Reader(Config.BinaryArch, &In);
   std::unique_ptr<Object> Obj = Reader.create();
 
   // 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);
+  if (Error E = handleArgs(Config, *Obj, Reader, OutputElfType))
+    return E;
   std::unique_ptr<Writer> Writer =
       createWriter(Config, *Obj, Out, OutputElfType);
   if (Error E = Writer->finalize())
-    error(std::move(E));
-  if (Error E = Writer->write())
-    error(std::move(E));
+    return E;
+  return Writer->write();
 }
 
-void executeObjcopyOnBinary(const CopyConfig &Config,
+Error 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 =
       Config.OutputArch ? getOutputElfType(Config.OutputArch.getValue())
                         : getOutputElfType(In);
   ArrayRef<uint8_t> BuildIdBytes;
 
   if (!Config.BuildIdLinkDir.empty()) {
     BuildIdBytes = unwrapOrError(findBuildID(In));
     if (BuildIdBytes.size() < 2)
-      error("build ID in file '" + Config.InputFilename +
-            "' is smaller than two bytes");
+      return createFileError(
+          Config.InputFilename,
+          createStringError(object_error::parse_failed,
+                            "build ID is smaller than two bytes."));
   }
 
   if (!Config.BuildIdLinkDir.empty() && Config.BuildIdLinkInput) {
     linkToBuildIdDir(Config, Config.InputFilename,
                      Config.BuildIdLinkInput.getValue(), BuildIdBytes);
   }
-  handleArgs(Config, *Obj, Reader, OutputElfType);
+  if (Error E = handleArgs(Config, *Obj, Reader, OutputElfType))
+    return E;
   std::unique_ptr<Writer> Writer =
       createWriter(Config, *Obj, Out, OutputElfType);
   if (Error E = Writer->finalize())
-    error(std::move(E));
+    return E;
   if (Error E = Writer->write())
-    error(std::move(E));
+    return E;
   if (!Config.BuildIdLinkDir.empty() && Config.BuildIdLinkOutput) {
     linkToBuildIdDir(Config, Config.OutputFilename,
                      Config.BuildIdLinkOutput.getValue(), BuildIdBytes);
   }
+  return Error::success();
 }
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm

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

for (const NewArchiveMember &Member : NewMembers) {
     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,
+static Error executeObjcopyOnRawBinary(const CopyConfig &Config,
                                        MemoryBuffer &In, Buffer &Out) {
   // TODO: llvm-objcopy should parse CopyConfig.OutputFormat to recognize
   // formats other than ELF / "binary" and invoke
   // elf::executeObjcopyOnRawBinary, macho::executeObjcopyOnRawBinary or
   // coff::executeObjcopyOnRawBinary accordingly.
   return elf::executeObjcopyOnRawBinary(Config, In, Out);
 }
 
 /// The function executeObjcopyOnBinary does the dispatch based on the format
 /// of the input binary (ELF, MachO or COFF).
-static void executeObjcopyOnBinary(const CopyConfig &Config, object::Binary &In,
-                                   Buffer &Out) {
+static Error executeObjcopyOnBinary(const CopyConfig &Config,
+                                    object::Binary &In, Buffer &Out) {
   if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In))
     return elf::executeObjcopyOnBinary(Config, *ELFBinary, Out);
   else if (auto *COFFBinary = dyn_cast<object::COFFObjectFile>(&In))
     return coff::executeObjcopyOnBinary(Config, *COFFBinary, Out);
   else
-    error("Unsupported object file format");
+    return createStringError(object_error::invalid_file_type,
+                             "Unsupported object file format");
 }
 
-static void executeObjcopyOnArchive(const CopyConfig &Config,
+static Error executeObjcopyOnArchive(const CopyConfig &Config,
                                      const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
   Error Err = Error::success();
   for (const Archive::Child &Child : Ar.children(Err)) {
     Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
     if (!ChildOrErr)
       reportError(Ar.getFileName(), ChildOrErr.takeError());
     Binary *Bin = ChildOrErr->get();
 
     Expected<StringRef> ChildNameOrErr = Child.getName();
     if (!ChildNameOrErr)
       reportError(Ar.getFileName(), ChildNameOrErr.takeError());
 
     MemBuffer MB(ChildNameOrErr.get());
-    executeObjcopyOnBinary(Config, *Bin, MB);
+    if (Error E = executeObjcopyOnBinary(Config, *Bin, MB))
+      return E;
 
     Expected<NewArchiveMember> Member =
         NewArchiveMember::getOldMember(Child, Config.DeterministicArchives);
     if (!Member)
       reportError(Ar.getFileName(), Member.takeError());
     Member->Buf = MB.releaseMemoryBuffer();
     Member->MemberName = Member->Buf->getBufferIdentifier();
     NewArchiveMembers.push_back(std::move(*Member));
   }
 
   if (Err)
     reportError(Config.InputFilename, std::move(Err));
   if (Error E = deepWriteArchive(Config.OutputFilename, NewArchiveMembers,
                                  Ar.hasSymbolTable(), Ar.kind(),
                                  Config.DeterministicArchives, Ar.isThin()))
     reportError(Config.OutputFilename, std::move(E));
+  return Error::success();
 }
 
 static void restoreDateOnFile(StringRef Filename,
                               const sys::fs::file_status &Stat) {
   int FD;
 
   if (auto EC =
           sys::fs::openFileForWrite(Filename, FD, sys::fs::CD_OpenExisting))
if (Config.PreserveDates)
     if (auto EC = sys::fs::status(Config.InputFilename, Stat))
       reportError(Config.InputFilename, EC);
 
   if (Config.InputFormat == "binary") {
     auto BufOrErr = MemoryBuffer::getFile(Config.InputFilename);
     if (!BufOrErr)
       reportError(Config.InputFilename, BufOrErr.getError());
     FileBuffer FB(Config.OutputFilename);
-    executeObjcopyOnRawBinary(Config, *BufOrErr->get(), FB);
+    if (Error E = executeObjcopyOnRawBinary(Config, *BufOrErr->get(), FB))
+      error(std::move(E));
   } else {
     Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
         createBinary(Config.InputFilename);
     if (!BinaryOrErr)
       reportError(Config.InputFilename, BinaryOrErr.takeError());
 
     if (Archive *Ar = dyn_cast<Archive>(BinaryOrErr.get().getBinary())) {
-      executeObjcopyOnArchive(Config, *Ar);
+      if (Error E = executeObjcopyOnArchive(Config, *Ar))
+        error(std::move(E));
     } else {
       FileBuffer FB(Config.OutputFilename);
-      executeObjcopyOnBinary(Config, *BinaryOrErr.get().getBinary(), FB);
+      if (Error E = executeObjcopyOnBinary(Config,
+                                           *BinaryOrErr.get().getBinary(), FB))
+        error(std::move(E));
     }
   }
 
   if (Config.PreserveDates) {
     restoreDateOnFile(Config.OutputFilename, Stat);
     if (!Config.SplitDWO.empty())
       restoreDateOnFile(Config.SplitDWO, Stat);
   }