[llvm-objcopy][NFC] refactor error handling. part 1.

[avl]

Summary

Remove usages of special error reporting functions(error(),
reportError()). This patch is extracted from D87987.
Errors are reported as Expected<>/Error returning values.
This part is for MachO subfolder of llvm-objcopy.

Testing: check-all.

[jhenderson]

llvm/tools/llvm-objcopy/CopyConfig.h

239–242 ↗

(On Diff #293556)

Why did this code change? It looks semantically identical, and I think it's actually a little less readable.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

381

Cleaner code would be to just do:

Expected<std::unique_ptr<Object>> O = Reader.create();
if (!ObjOrErr)
  return O.takeError();
...

383–386

If I'm following things correctly, this piece of code is dead? Even if there were errors in the create method, they should now be reported via the Expected being returned. If it is dead, please remove it. If not, I think we should change the behaviour to report the error in the same manner as the other errors.

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

27–42 ↗

(On Diff #293556)

If I'm not mistaken, this whole function can just be replaced with a single createStringError() call, similar to what we often do in llvm-readobj (see e.g. https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L5348).

[avl]

llvm/tools/llvm-objcopy/CopyConfig.h

239–242 ↗

(On Diff #293556)

I personally found this way more readable. I used it elsewhere and changed in this place so that it looks equal to other places. Will change back.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

381

Ok, will use that pattern.

383–386

yes, it looks dead. I left it just in case. Will remove.

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

27–42 ↗

(On Diff #293556)

OK.

[avl]

addressed comments.

[jhenderson]

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

370

I think you don't need the asser. A lot of our code base don't bother asserting that something is not null, because it's not really any different from a crash in that situation.

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

Can you use the error code associated with SecRef instead of errc::invalid_argument here?

135–141

Here and below in these case statements, due to the scoping issue, I'm okay if you want to go with your previous version.

[avl]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

As far as I understand that could be done in that way:

inline Error errorWithContext(StringRef Context, Error E) {
  assert(E);
  std::string Buf;
  raw_string_ostream OS(Buf);
  Optional<std::error_code> EC;
  handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
    OS << "'" + Context + "': ";
    if (!EC)
      EC = EI.convertToErrorCode();
    EI.log(OS);
    OS << "\n";
  });
  OS.flush();

  return createStringError(*EC, Buf);
}

but you`ve asked me to not create a special routine for that.

[jhenderson]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

I misremembered how the interface worked. There's an errorToErrorCode() function, but that consumes the input Error, which we don't want. Leave it as-is.

[avl]

addressed comments.

[jhenderson]

Looks good from my point of view. Might be worth a second opinion (@grimar/@MaskRay/@rupprecht/@alexshap?)

[alexander-shaposhnikov]

apart from the minor comments, I think this change is in the right direction.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

367

nit: perhaps, I'd keep the old name of the variable (simply O) because ObjOrErr somehow appears to suggest that the type of this variable is ErrorOr<...> (https://llvm.org/doxygen/classllvm_1_1ErrorOr.html ) which is not the case.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

I'm wondering - why not simply bubble up the error ?

127–132

the same as above - the name SectionsOrErr appears to be slightly misleading, I'd probably just call it Sections.

[avl]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

My guess is that original intention is to add file name MachOObj.getFileName to have better reporting. If we would like to add filename then we could not bubble up original error.

[avl]

renamed variables.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

On the line 359 (in MachOObjcopy.cpp) we can use createFileError (similarly to how we deal with handleArgs) and this would probably enable us to get rid of these conversions / the code would simplify a bit.
P.S. since we were not propagating errors before this change we had to adjust the message in multiple places
what would you say to this ?

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

91–97

DataOrErr -> Data or Content

[avl]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

Ah, you mean to use createFileError instead of createString Error. Right, that would be better usage.

As to adjusting the message in multiple places - did you mean checking other usages of createStringError and replace them with createFileError if applicable ? It looks like all other usages of createStringError inside MachO subfolder are not suitable to replacing with createFileError.

[avl]

replaced createStringError with createFileError, renamed variable.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

I was referring to the lines 359/360 in MachOObjcopy.cpp

So here you'd simply return Data.takeError() etc,
and in MachOObjcopy.cpp we would have

Expected<std::unique_ptr<Object>> O = Reader.create();
if (!O)
  return createFileError(Config.InputFilename, O.takeError());

if (Error E = handleArgs(Config, **O))
  return createFileError(Config.InputFilename, std::move(E));

[avl]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

89

got it, thanks.

[avl]

move usage of createFileError at high level.

[alexander-shaposhnikov]

LG, thanks

[grimar]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

91–97

I think DataOrErr was better. Searching of "OrErr" in LLVM code reveals many places (includeing llvm-objcopy code) where such suffic is used for "Expected<>".

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

if (Error E = processLoadCommands(Config, Obj))
     return E;
 
   return Error::success();
 }
 
 Error executeObjcopyOnBinary(const CopyConfig &Config,
                              object::MachOObjectFile &In, Buffer &Out) {
   MachOReader Reader(In);
-  std::unique_ptr<Object> O = Reader.create();
+  Expected<std::unique_ptr<Object>> O = Reader.create();
   if (!O)
-    return createFileError(
-        Config.InputFilename,
-        createStringError(object_error::parse_failed,
-                          "unable to deserialize MachO object"));
+    return createFileError(Config.InputFilename, O.takeError());
 
-  if (Error E = handleArgs(Config, *O))
+  if (Error E = handleArgs(Config, **O))
     return createFileError(Config.InputFilename, std::move(E));
 
   // Page size used for alignment of segment sizes in Mach-O executables and
   // dynamic libraries.
   uint64_t PageSize;
   switch (In.getArch()) {
   case Triple::ArchType::arm:
   case Triple::ArchType::aarch64:
   case Triple::ArchType::aarch64_32:
     PageSize = 16384;
     break;
   default:
     PageSize = 4096;
   }
 
-  MachOWriter Writer(*O, In.is64Bit(), In.isLittleEndian(), PageSize, Out);
+  MachOWriter Writer(**O, In.is64Bit(), In.isLittleEndian(), PageSize, Out);
   if (auto E = Writer.finalize())
     return E;
   return Writer.write();
 }
 
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/MachO/MachOReader.h

 namespace objcopy {
 namespace macho {
 
 // The hierarchy of readers is responsible for parsing different inputs:
 // raw binaries and regular MachO object files.
 class Reader {
 public:
   virtual ~Reader(){};
-  virtual std::unique_ptr<Object> create() const = 0;
+  virtual Expected<std::unique_ptr<Object>> create() const = 0;
 };
 
 class MachOReader : public Reader {
   const object::MachOObjectFile &MachOObj;
 
   void readHeader(Object &O) const;
-  void readLoadCommands(Object &O) const;
+  Error readLoadCommands(Object &O) const;
   void readSymbolTable(Object &O) const;
   void setSymbolInRelocationInfo(Object &O) const;
   void readRebaseInfo(Object &O) const;
   void readBindInfo(Object &O) const;
   void readWeakBindInfo(Object &O) const;
   void readLazyBindInfo(Object &O) const;
   void readExportInfo(Object &O) const;
   void readLinkData(Object &O, Optional<size_t> LCIndex, LinkData &LD) const;
   void readCodeSignature(Object &O) const;
   void readDataInCodeData(Object &O) const;
   void readFunctionStartsData(Object &O) const;
   void readIndirectSymbolTable(Object &O) const;
   void readSwiftVersion(Object &O) const;
 
 public:
   explicit MachOReader(const object::MachOObjectFile &Obj) : MachOObj(Obj) {}
 
-  std::unique_ptr<Object> create() const override;
+  Expected<std::unique_ptr<Object>> create() const override;
 };
 
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

 //===- MachOReader.cpp ------------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "MachOReader.h"
 #include "../llvm-objcopy.h"
 #include "Object.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/Object/MachO.h"
+#include "llvm/Support/Errc.h"
 #include <memory>
 
 namespace llvm {
 namespace objcopy {
 namespace macho {
 
 void MachOReader::readHeader(Object &O) const {
   O.Header.Magic = MachOObj.getHeader().magic;

 }
 
 template <> Section constructSection(MachO::section_64 Sec, uint32_t Index) {
   Section S = constructSectionCommon(Sec, Index);
   S.Reserved3 = Sec.reserved3;
   return S;
 }
 
-// TODO: get rid of reportError and make MachOReader return Expected<> instead.
 template <typename SectionType, typename SegmentType>
-std::vector<std::unique_ptr<Section>>
+Expected<std::vector<std::unique_ptr<Section>>>
 extractSections(const object::MachOObjectFile::LoadCommandInfo &LoadCmd,
                 const object::MachOObjectFile &MachOObj,
                 uint32_t &NextSectionIndex) {
   auto End = LoadCmd.Ptr + LoadCmd.C.cmdsize;
   const SectionType *Curr =
       reinterpret_cast<const SectionType *>(LoadCmd.Ptr + sizeof(SegmentType));
   std::vector<std::unique_ptr<Section>> Sections;
   for (; reinterpret_cast<const void *>(Curr) < End; Curr++) {
     if (MachOObj.isLittleEndian() != sys::IsLittleEndianHost) {
       SectionType Sec;
       memcpy((void *)&Sec, Curr, sizeof(SectionType));
       MachO::swapStruct(Sec);
       Sections.push_back(
           std::make_unique<Section>(constructSection(Sec, NextSectionIndex)));
     } else {
       Sections.push_back(
           std::make_unique<Section>(constructSection(*Curr, NextSectionIndex)));
     }
 
     Section &S = *Sections.back();
 
     Expected<object::SectionRef> SecRef =
         MachOObj.getSection(NextSectionIndex++);
     if (!SecRef)
-      reportError(MachOObj.getFileName(), SecRef.takeError());
+      return SecRef.takeError();
+
+    Expected<ArrayRef<uint8_t>> Data =
+        MachOObj.getSectionContents(SecRef->getRawDataRefImpl());
+    if (!Data)
+      return Data.takeError();
 
-    if (Expected<ArrayRef<uint8_t>> E =
-            MachOObj.getSectionContents(SecRef->getRawDataRefImpl()))
-      S.Content =
-          StringRef(reinterpret_cast<const char *>(E->data()), E->size());
-    else
-      reportError(MachOObj.getFileName(), E.takeError());
+        StringRef(reinterpret_cast<const char *>(Data->data()), Data->size());
 
     S.Relocations.reserve(S.NReloc);
     for (auto RI = MachOObj.section_rel_begin(SecRef->getRawDataRefImpl()),
               RE = MachOObj.section_rel_end(SecRef->getRawDataRefImpl());
          RI != RE; ++RI) {
       RelocationInfo R;
       R.Symbol = nullptr; // We'll fill this field later.
       R.Info = MachOObj.getRelocation(RI->getRawDataRefImpl());
       R.Scattered = MachOObj.isRelocationScattered(R.Info);
       R.Extern = !R.Scattered && MachOObj.getPlainRelocationExternal(R.Info);
       S.Relocations.push_back(R);
     }
 
     assert(S.NReloc == S.Relocations.size() &&
            "Incorrect number of relocations");
   }
   return Sections;
 }
 
-void MachOReader::readLoadCommands(Object &O) const {
+Error MachOReader::readLoadCommands(Object &O) const {
   // For MachO sections indices start from 1.
   uint32_t NextSectionIndex = 1;
   for (auto LoadCmd : MachOObj.load_commands()) {
     LoadCommand LC;
     switch (LoadCmd.C.cmd) {
     case MachO::LC_CODE_SIGNATURE:
       O.CodeSignatureCommandIndex = O.LoadCommands.size();
       break;
     case MachO::LC_SEGMENT:
-      LC.Sections = extractSections<MachO::section, MachO::segment_command>(
-          LoadCmd, MachOObj, NextSectionIndex);
+      if (Expected<std::vector<std::unique_ptr<Section>>> Sections =
+              extractSections<MachO::section, MachO::segment_command>(
+                  LoadCmd, MachOObj, NextSectionIndex))
+        LC.Sections = std::move(*Sections);
+      else
+        return Sections.takeError();
       break;
     case MachO::LC_SEGMENT_64:
-      LC.Sections =
+      if (Expected<std::vector<std::unique_ptr<Section>>> Sections =
               extractSections<MachO::section_64, MachO::segment_command_64>(
-              LoadCmd, MachOObj, NextSectionIndex);
+                  LoadCmd, MachOObj, NextSectionIndex))
+        LC.Sections = std::move(*Sections);
+      else
+        return Sections.takeError();
       break;
     case MachO::LC_SYMTAB:
       O.SymTabCommandIndex = O.LoadCommands.size();
       break;
     case MachO::LC_DYSYMTAB:
       O.DySymTabCommandIndex = O.LoadCommands.size();
       break;
     case MachO::LC_DYLD_INFO:
     case MachO::LC_DYLD_INFO_ONLY:
default:
             reinterpret_cast<uint8_t *>(const_cast<char *>(LoadCmd.Ptr)) +
                 sizeof(MachO::load_command),
             LoadCmd.C.cmdsize - sizeof(MachO::load_command));
       break;
 #include "llvm/BinaryFormat/MachO.def"
     }
     O.LoadCommands.push_back(std::move(LC));
   }
+  return Error::success();
 }
 
 template <typename nlist_t>
 SymbolEntry constructSymbolEntry(StringRef StrTable, const nlist_t &nlist) {
   assert(nlist.n_strx < StrTable.size() &&
          "n_strx exceeds the size of the string table");
   SymbolEntry SE;
   SE.Name = StringRef(StrTable.data() + nlist.n_strx).str();
for (const std::unique_ptr<Section> &Sec : LC.Sections)
           sys::swapByteOrder(ImageInfo.Version);
           sys::swapByteOrder(ImageInfo.Flags);
         }
         O.SwiftVersion = (ImageInfo.Flags >> 8) & 0xff;
         return;
       }
 }
 
-std::unique_ptr<Object> MachOReader::create() const {
+Expected<std::unique_ptr<Object>> MachOReader::create() const {
   auto Obj = std::make_unique<Object>();
   readHeader(*Obj);
-  readLoadCommands(*Obj);
+  if (Error E = readLoadCommands(*Obj))
+    return std::move(E);
   readSymbolTable(*Obj);
   setSymbolInRelocationInfo(*Obj);
   readRebaseInfo(*Obj);
   readBindInfo(*Obj);
   readWeakBindInfo(*Obj);
   readLazyBindInfo(*Obj);
   readExportInfo(*Obj);
   readCodeSignature(*Obj);