[llvm-objcopy] [COFF] Use Error/Expected returns instead of calling reportError. NFC.

[alexander-shaposhnikov]

I think this is a step in the right direction, although now we have some inconsistency with the existing code, but okay, I hope we will change the old code in the future as well. Plus I will change the not yet committed code for MachO myself.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 namespace coff {
 
 using namespace object;
 using namespace COFF;
 
 void executeObjcopyOnBinary(const CopyConfig &Config,
                             object::COFFObjectFile &In, Buffer &Out) {
   COFFReader Reader(In);
-  std::unique_ptr<Object> Obj = Reader.create();
+  Expected<std::unique_ptr<Object>> ObjOrErr = Reader.create();
+  if (!ObjOrErr)
+    reportError(Config.InputFilename, ObjOrErr.takeError());
+  Object *Obj = ObjOrErr->get();
   assert(Obj && "Unable to deserialize COFF object");
   COFFWriter Writer(*Obj, Out);
-  Writer.write();
+  if (Error E = Writer.write())
+    reportError(Config.OutputFilename, std::move(E));
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

tools/llvm-objcopy/COFF/Reader.h

 //===- Reader.h -------------------------------------------------*- C++ -*-===//
 //
 //                      The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_OBJCOPY_COFF_READER_H
 #define LLVM_TOOLS_OBJCOPY_COFF_READER_H
 
 #include "Buffer.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/Object/COFF.h"
+#include "llvm/Support/Error.h"
 
 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 struct Object;
 
 using object::COFFObjectFile;
 
 class Reader {
 public:
   virtual ~Reader();
-  virtual std::unique_ptr<Object> create() const = 0;
+  virtual Expected<std::unique_ptr<Object>> create() const = 0;
 };
 
 class COFFReader : public Reader {
   const COFFObjectFile &COFFObj;
 
-  void readExecutableHeaders(Object &Obj) const;
-  void readSections(Object &Obj) const;
-  void readSymbols(Object &Obj, bool IsBigObj) const;
+  Error readExecutableHeaders(Object &Obj) const;
+  Error readSections(Object &Obj) const;
+  Error readSymbols(Object &Obj, bool IsBigObj) const;
 
 public:
   explicit COFFReader(const COFFObjectFile &O) : COFFObj(O) {}
-  std::unique_ptr<Object> create() const override;
+  Expected<std::unique_ptr<Object>> create() const override;
 };
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_COFF_READER_H

tools/llvm-objcopy/COFF/Reader.cpp

 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 using namespace object;
 
 Reader::~Reader() {}
 
-void COFFReader::readExecutableHeaders(Object &Obj) const {
+Error COFFReader::readExecutableHeaders(Object &Obj) const {
   const dos_header *DH = COFFObj.getDOSHeader();
   Obj.Is64 = COFFObj.is64();
   if (!DH)
-    return;
+    return Error::success();
 
   Obj.IsPE = true;
   Obj.DosHeader = *DH;
   if (DH->AddressOfNewExeHeader > sizeof(*DH))
     Obj.DosStub = ArrayRef<uint8_t>(reinterpret_cast<const uint8_t *>(&DH[1]),
                                     DH->AddressOfNewExeHeader - sizeof(*DH));
 
   if (COFFObj.is64()) {
     const pe32plus_header *PE32Plus = nullptr;
     if (auto EC = COFFObj.getPE32PlusHeader(PE32Plus))
-      reportError(COFFObj.getFileName(), std::move(EC));
+      return errorCodeToError(EC);
     Obj.PeHeader = *PE32Plus;
   } else {
     const pe32_header *PE32 = nullptr;
     if (auto EC = COFFObj.getPE32Header(PE32))
-      reportError(COFFObj.getFileName(), std::move(EC));
+      return errorCodeToError(EC);
     copyPeHeader(Obj.PeHeader, *PE32);
     // The pe32plus_header (stored in Object) lacks the BaseOfData field.
     Obj.BaseOfData = PE32->BaseOfData;
   }
 
   for (size_t I = 0; I < Obj.PeHeader.NumberOfRvaAndSize; I++) {
     const data_directory *Dir;
     if (auto EC = COFFObj.getDataDirectory(I, Dir))
-      reportError(COFFObj.getFileName(), std::move(EC));
+      return errorCodeToError(EC);
     Obj.DataDirectories.emplace_back(*Dir);
   }
+  return Error::success();
 }
 
-void COFFReader::readSections(Object &Obj) const {
+Error COFFReader::readSections(Object &Obj) const {
   // Section indexing starts from 1.
   for (size_t I = 1, E = COFFObj.getNumberOfSections(); I <= E; I++) {
     const coff_section *Sec;
     if (auto EC = COFFObj.getSection(I, Sec))
-      reportError(COFFObj.getFileName(), std::move(EC));
+      return errorCodeToError(EC);
     Obj.Sections.push_back(Section());
     Section &S = Obj.Sections.back();
     S.Header = *Sec;
     if (auto EC = COFFObj.getSectionContents(Sec, S.Contents))
-      reportError(COFFObj.getFileName(), std::move(EC));
+      return errorCodeToError(EC);
     ArrayRef<coff_relocation> Relocs = COFFObj.getRelocations(Sec);
-    S.Relocs.insert(S.Relocs.end(), Relocs.begin(), Relocs.end());
+    for (const coff_relocation &R : Relocs)
+      S.Relocs.push_back(R);
     if (auto EC = COFFObj.getSectionName(Sec, S.Name))
-      reportError(COFFObj.getFileName(), std::move(EC));
+      return errorCodeToError(EC);
     if (Sec->hasExtendedRelocations())
-      reportError(
-          COFFObj.getFileName(),
-          make_error<StringError>("Extended relocations not supported yet",
-                                  object_error::parse_failed));
+      return make_error<StringError>("Extended relocations not supported yet",
+                                     object_error::parse_failed);
   }
+  return Error::success();
 }
 
-void COFFReader::readSymbols(Object &Obj, bool IsBigObj) const {
+Error COFFReader::readSymbols(Object &Obj, bool IsBigObj) const {
   for (uint32_t I = 0, E = COFFObj.getRawNumberOfSymbols(); I < E;) {
     Expected<COFFSymbolRef> SymOrErr = COFFObj.getSymbol(I);
     if (!SymOrErr)
-      reportError(COFFObj.getFileName(), SymOrErr.takeError());
+      return SymOrErr.takeError();
     COFFSymbolRef SymRef = *SymOrErr;
 
     Obj.Symbols.push_back(Symbol());
     Symbol &Sym = Obj.Symbols.back();
     // Copy symbols from the original form into an intermediate coff_symbol32.
     if (IsBigObj)
       copySymbol(Sym.Sym,
                  *reinterpret_cast<const coff_symbol32 *>(SymRef.getRawPtr()));
     else
       copySymbol(Sym.Sym,
                  *reinterpret_cast<const coff_symbol16 *>(SymRef.getRawPtr()));
     if (auto EC = COFFObj.getSymbolName(SymRef, Sym.Name))
-      reportError(COFFObj.getFileName(), std::move(EC));
+      return errorCodeToError(EC);
     Sym.AuxData = COFFObj.getSymbolAuxData(SymRef);
     assert((Sym.AuxData.size() %
             (IsBigObj ? sizeof(coff_symbol32) : sizeof(coff_symbol16))) == 0);
     I += 1 + Sym.Sym.NumberOfAuxSymbols;
   }
+  return Error::success();
 }
 
-std::unique_ptr<Object> COFFReader::create() const {
+Expected<std::unique_ptr<Object>> COFFReader::create() const {
   auto Obj = llvm::make_unique<Object>();
 
   const coff_file_header *CFH = nullptr;
   const coff_bigobj_file_header *CBFH = nullptr;
   COFFObj.getCOFFHeader(CFH);
   COFFObj.getCOFFBigObjHeader(CBFH);
   bool IsBigObj = false;
   if (CFH) {
     Obj->CoffFileHeader = *CFH;
   } else {
     if (!CBFH)
-      reportError(COFFObj.getFileName(),
-                  make_error<StringError>("No COFF file header returned",
-                                          object_error::parse_failed));
+      return make_error<StringError>("No COFF file header returned",
+                                     object_error::parse_failed);
     // Only copying the few fields from the bigobj header that we need
     // and won't recreate in the end.
     Obj->CoffFileHeader.Machine = CBFH->Machine;
     Obj->CoffFileHeader.TimeDateStamp = CBFH->TimeDateStamp;
     IsBigObj = true;
   }
 
-  readExecutableHeaders(*Obj);
-  readSections(*Obj);
-  readSymbols(*Obj, IsBigObj);
+  if (Error E = readExecutableHeaders(*Obj))
+    return std::move(E);
+  if (Error E = readSections(*Obj))
+    return std::move(E);
+  if (Error E = readSymbols(*Obj, IsBigObj))
+    return std::move(E);
 
-  return Obj;
+  return std::move(Obj);
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

tools/llvm-objcopy/COFF/Writer.h

 //===- Writer.h -------------------------------------------------*- C++ -*-===//
 //
 //                      The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_OBJCOPY_COFF_WRITER_H
 #define LLVM_TOOLS_OBJCOPY_COFF_WRITER_H
 
 #include "Buffer.h"
 #include "llvm/MC/StringTableBuilder.h"
+#include "llvm/Support/Error.h"
 #include <cstddef>
 #include <utility>
 
 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 struct Object;
 
 class Writer {
 protected:
   Object &Obj;
   Buffer &Buf;
 
 public:
   virtual ~Writer();
-  virtual void write() = 0;
+  virtual Error write() = 0;
 
   Writer(Object &O, Buffer &B) : Obj(O), Buf(B) {}
 };
 
 class COFFWriter : public Writer {
   size_t FileSize;
   size_t FileAlignment;
   size_t SizeOfInitializedData;
   StringTableBuilder StrTabBuilder;
 
   void layoutSections();
   size_t finalizeStringTable();
   template <class SymbolTy> std::pair<size_t, size_t> finalizeSymbolTable();
 
   void finalize(bool IsBigObj);
 
   void writeHeaders(bool IsBigObj);
   void writeSections();
   template <class SymbolTy> void writeSymbolStringTables();
 
-  void write(bool IsBigObj);
+  Error write(bool IsBigObj);
 
-  void patchDebugDirectory();
+  Error patchDebugDirectory();
 
 public:
   virtual ~COFFWriter() {}
-  void write() override;
+  Error write() override;
 
   COFFWriter(Object &Obj, Buffer &Buf)
       : Writer(Obj, Buf), StrTabBuilder(StringTableBuilder::WinCOFF) {}
 };
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_COFF_WRITER_H

tools/llvm-objcopy/COFF/Writer.cpp

template <class SymbolTy> void COFFWriter::writeSymbolStringTables() {
   }
   if (StrTabBuilder.getSize() > 4 || !Obj.IsPE) {
     // Always write a string table in object files, even an empty one.
     StrTabBuilder.write(Ptr);
     Ptr += StrTabBuilder.getSize();
   }
 }
 
-void COFFWriter::write(bool IsBigObj) {
+Error COFFWriter::write(bool IsBigObj) {
   finalize(IsBigObj);
 
   Buf.allocate(FileSize);
 
   writeHeaders(IsBigObj);
   writeSections();
   if (IsBigObj)
     writeSymbolStringTables<coff_symbol32>();
   else
     writeSymbolStringTables<coff_symbol16>();
 
   if (Obj.IsPE)
-    patchDebugDirectory();
+    if (Error E = patchDebugDirectory())
+      return std::move(E);
 
-  if (auto E = Buf.commit())
-    reportError(Buf.getName(), errorToErrorCode(std::move(E)));
+  return Buf.commit();
 }
 
 // Locate which sections contain the debug directories, iterate over all
 // the debug_directory structs in there, and set the PointerToRawData field
 // in all of them, according to their new physical location in the file.
-void COFFWriter::patchDebugDirectory() {
+Error COFFWriter::patchDebugDirectory() {
   if (Obj.DataDirectories.size() < DEBUG_DIRECTORY)
-    return;
+    return Error::success();
   const data_directory *Dir = &Obj.DataDirectories[DEBUG_DIRECTORY];
   if (Dir->Size <= 0)
-    return;
+    return Error::success();
   for (const auto &S : Obj.Sections) {
     if (Dir->RelativeVirtualAddress >= S.Header.VirtualAddress &&
         Dir->RelativeVirtualAddress <
             S.Header.VirtualAddress + S.Header.SizeOfRawData) {
       if (Dir->RelativeVirtualAddress + Dir->Size >
           S.Header.VirtualAddress + S.Header.SizeOfRawData)
-        reportError(Buf.getName(),
-                    make_error<StringError>(
-                        "Debug directory extends past end of section",
-                        object_error::parse_failed));
+        return make_error<StringError>(
+            object_error::parse_failed);
 
       size_t Offset = Dir->RelativeVirtualAddress - S.Header.VirtualAddress;
       uint8_t *Ptr = Buf.getBufferStart() + S.Header.PointerToRawData + Offset;
       uint8_t *End = Ptr + Dir->Size;
       while (Ptr < End) {
         debug_directory *Debug = reinterpret_cast<debug_directory *>(Ptr);
         Debug->PointerToRawData =
             S.Header.PointerToRawData + Offset + sizeof(debug_directory);
         Ptr += sizeof(debug_directory) + Debug->SizeOfData;
         Offset += sizeof(debug_directory) + Debug->SizeOfData;
       }
       // Debug directory found and patched, all done.
-      return;
+      return Error::success();
     }
   }
-  reportError(Buf.getName(),
-              make_error<StringError>("Debug directory not found",
-                                      object_error::parse_failed));
+  return make_error<StringError>("Debug directory not found",
+                                 object_error::parse_failed);
 }
 
-void COFFWriter::write() {
+Error COFFWriter::write() {
   bool IsBigObj = Obj.Sections.size() > MaxNumberOfSections16;
   if (IsBigObj && Obj.IsPE)
-    reportError(Buf.getName(),
-                make_error<StringError>("Too many sections for executable",
-                                        object_error::parse_failed));
-  write(IsBigObj);
+    return make_error<StringError>("Too many sections for executable",
+                                   object_error::parse_failed);
+  return write(IsBigObj);
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm