[Object] Change Archive::child_iterator for better interop with Error/Expected.

See http://reviews.llvm.org/D22079

Changes the Archive::child_begin and Archive::children to require a reference
to an Error. If iterator increment fails (because the archive header is
damaged) the iterator will be set to 'end()', and the error stored in the
given Error&. The Error value should be checked by the user immediately after
the loop. E.g.:

Error Err;
for (auto &C : A->children(Err)) {
  // Do something with archive child C.
}
// Check the error immediately after the loop.
if (Err)
  return Err;

Failure to check the Error will result in an abort() when the Error goes out of
scope (as guaranteed by the Error class).

llvm-svn: 275316

Author: lhames

llvm/include/llvm/Object/Archive.h

@@ -106,21 +106,20 @@ class Archive : public Binary {
   };
 
   class child_iterator {
-    ErrorOr<Child> child;
+    Child C;
+    Error *E;
 
   public:
-    child_iterator() : child(Child(nullptr, nullptr, nullptr)) {}
-    child_iterator(const Child &c) : child(c) {}
-    child_iterator(std::error_code EC) : child(EC) {}
-    const ErrorOr<Child> *operator->() const { return &child; }
-    const ErrorOr<Child> &operator*() const { return child; }
+    child_iterator() : C(Child(nullptr, nullptr, nullptr)), E(nullptr) {}
+    child_iterator(const Child &C, Error *E) : C(C), E(E) {}
+    const Child *operator->() const { return &C; }
+    const Child &operator*() const { return C; }
 
     bool operator==(const child_iterator &other) const {
-      // We ignore error states so that comparisions with end() work, which
-      // allows range loops.
-      if (child.getError() || other.child.getError())
-        return false;
-      return *child == *other.child;
+      // Ignore errors here: If an error occurred during increment then getNext
+      // will have been set to child_end(), and the following comparison should
+      // do the right thing.
+      return C == other.C;
     }
 
     bool operator!=(const child_iterator &other) const {
@@ -130,8 +129,15 @@ class Archive : public Binary {
     // Code in loops with child_iterators must check for errors on each loop
     // iteration.  And if there is an error break out of the loop.
     child_iterator &operator++() { // Preincrement
-      assert(child && "Can't increment iterator with error");
-      child = child->getNext();
+      assert(E && "Can't increment iterator with no Error attached");
+      if (auto ChildOrErr = C.getNext())
+        C = *ChildOrErr;
+      else {
+        ErrorAsOutParameter ErrAsOutParam(*E);
+        C = C.getParent()->child_end().C;
+        *E = errorCodeToError(ChildOrErr.getError());
+        E = nullptr;
+      }
       return *this;
     }
   };
@@ -190,10 +196,11 @@ class Archive : public Binary {
   Kind kind() const { return (Kind)Format; }
   bool isThin() const { return IsThin; }
 
-  child_iterator child_begin(bool SkipInternal = true) const;
+  child_iterator child_begin(Error &Err, bool SkipInternal = true) const;
   child_iterator child_end() const;
-  iterator_range<child_iterator> children(bool SkipInternal = true) const {
-    return make_range(child_begin(SkipInternal), child_end());
+  iterator_range<child_iterator> children(Error &Err,
+                                          bool SkipInternal = true) const {
+    return make_range(child_begin(Err, SkipInternal), child_end());
   }
 
   symbol_iterator symbol_begin() const;
@@ -208,7 +215,7 @@ class Archive : public Binary {
   }
 
   // check if a symbol is in the archive
-  child_iterator findSym(StringRef name) const;
+  child_iterator findSym(Error &Err, StringRef name) const;
 
   bool hasSymbolTable() const;
   StringRef getSymbolTable() const { return SymbolTable; }

llvm/include/llvm/Support/Error.h

@@ -940,6 +940,11 @@ class ExitOnError {
   std::function<int(const Error &)> GetExitCode;
 };
 
+/// Report a serious error, calling any installed error handler. See
+/// ErrorHandling.h.
+LLVM_ATTRIBUTE_NORETURN void report_fatal_error(Error Err,
+                                                bool gen_crash_diag = true);
+
 } // namespace llvm
 
 #endif // LLVM_SUPPORT_ERROR_H

llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp

@@ -327,13 +327,14 @@ RuntimeDyld::SymbolInfo MCJIT::findSymbol(const std::string &Name,
   for (object::OwningBinary<object::Archive> &OB : Archives) {
     object::Archive *A = OB.getBinary();
     // Look for our symbols in each Archive
-    object::Archive::child_iterator ChildIt = A->findSym(Name);
-    if (std::error_code EC = ChildIt->getError())
-      report_fatal_error(EC.message());
+    Error Err;
+    object::Archive::child_iterator ChildIt = A->findSym(Err, Name);
+    if (Err)
+      report_fatal_error(std::move(Err));
     if (ChildIt != A->child_end()) {
       // FIXME: Support nested archives?
       Expected<std::unique_ptr<object::Binary>> ChildBinOrErr =
-          (*ChildIt)->getAsBinary();
+          ChildIt->getAsBinary();
       if (!ChildBinOrErr) {
         // TODO: Actually report errors helpfully.
         consumeError(ChildBinOrErr.takeError());

llvm/lib/ExecutionEngine/Orc/OrcMCJITReplacement.h

@@ -258,13 +258,14 @@ class OrcMCJITReplacement : public ExecutionEngine {
     for (object::OwningBinary<object::Archive> &OB : Archives) {
       object::Archive *A = OB.getBinary();
       // Look for our symbols in each Archive
-      object::Archive::child_iterator ChildIt = A->findSym(Name);
-      if (std::error_code EC = ChildIt->getError())
-        report_fatal_error(EC.message());
+      Error Err;
+      object::Archive::child_iterator ChildIt = A->findSym(Err, Name);
+      if (Err)
+        report_fatal_error(std::move(Err));
       if (ChildIt != A->child_end()) {
         // FIXME: Support nested archives?
         Expected<std::unique_ptr<object::Binary>> ChildBinOrErr =
-            (*ChildIt)->getAsBinary();
+            ChildIt->getAsBinary();
         if (!ChildBinOrErr) {
           // TODO: Actually report errors helpfully.
           consumeError(ChildBinOrErr.takeError());

llvm/lib/Object/Archive.cpp

@@ -298,12 +298,9 @@ Archive::Archive(MemoryBufferRef Source, Error &Err)
   }
 
   // Get the special members.
-  child_iterator I = child_begin(false);
-  std::error_code ec;
-  if ((ec = I->getError())) {
-    Err = errorCodeToError(ec);
+  child_iterator I = child_begin(Err, false);
+  if (Err)
     return;
-  }
   child_iterator E = child_end();
 
   // This is at least a valid empty archive. Since an empty archive is the
@@ -315,13 +312,13 @@ Archive::Archive(MemoryBufferRef Source, Error &Err)
     Err = Error::success();
     return;
   }
-  const Child *C = &**I;
+  const Child *C = &*I;
 
   auto Increment = [&]() {
     ++I;
-    if ((Err = errorCodeToError(I->getError())))
+    if (Err)
       return true;
-    C = &**I;
+    C = &*I;
     return false;
   };
 
@@ -366,8 +363,7 @@ Archive::Archive(MemoryBufferRef Source, Error &Err)
     Format = K_BSD;
     // We know this is BSD, so getName will work since there is no string table.
     ErrorOr<StringRef> NameOrErr = C->getName();
-    ec = NameOrErr.getError();
-    if (ec) {
+    if (auto ec = NameOrErr.getError()) {
       Err = errorCodeToError(ec);
       return;
     }
@@ -465,23 +461,29 @@ Archive::Archive(MemoryBufferRef Source, Error &Err)
   Err = Error::success();
 }
 
-Archive::child_iterator Archive::child_begin(bool SkipInternal) const {
+Archive::child_iterator Archive::child_begin(Error &Err,
+                                             bool SkipInternal) const {
   if (Data.getBufferSize() == 8) // empty archive.
     return child_end();
 
   if (SkipInternal)
-    return Child(this, FirstRegularData, FirstRegularStartOfFile);
+    return child_iterator(Child(this, FirstRegularData,
+                                FirstRegularStartOfFile),
+                          &Err);
 
   const char *Loc = Data.getBufferStart() + strlen(Magic);
   std::error_code EC;
-  Child c(this, Loc, &EC);
-  if (EC)
-    return child_iterator(EC);
-  return child_iterator(c);
+  Child C(this, Loc, &EC);
+  if (EC) {
+    ErrorAsOutParameter ErrAsOutParam(Err);
+    Err = errorCodeToError(EC);
+    return child_end();
+  }
+  return child_iterator(C, &Err);
 }
 
 Archive::child_iterator Archive::child_end() const {
-  return Child(this, nullptr, nullptr);
+  return child_iterator(Child(this, nullptr, nullptr), nullptr);
 }
 
 StringRef Archive::Symbol::getName() const {
@@ -665,18 +667,20 @@ uint32_t Archive::getNumberOfSymbols() const {
   return read32le(buf);
 }
 
-Archive::child_iterator Archive::findSym(StringRef name) const {
+Archive::child_iterator Archive::findSym(Error &Err, StringRef name) const {
   Archive::symbol_iterator bs = symbol_begin();
   Archive::symbol_iterator es = symbol_end();
 
   for (; bs != es; ++bs) {
     StringRef SymName = bs->getName();
     if (SymName == name) {
-      ErrorOr<Archive::child_iterator> ResultOrErr = bs->getMember();
-      // FIXME: Should we really eat the error?
-      if (ResultOrErr.getError())
+      if (auto MemberOrErr = bs->getMember()) {
+        return child_iterator(*MemberOrErr, &Err);
+      } else {
+        ErrorAsOutParameter ErrAsOutParam(Err);
+        Err = errorCodeToError(MemberOrErr.getError());
         return child_end();
-      return ResultOrErr.get();
+      }
     }
   }
   return child_end();

llvm/lib/Support/Error.cpp

@@ -64,6 +64,7 @@ void logAllUnhandledErrors(Error E, raw_ostream &OS, Twine ErrorBanner) {
   });
 }
 
+
 std::error_code ErrorList::convertToErrorCode() const {
   return std::error_code(static_cast<int>(ErrorErrorCode::MultipleErrors),
                          *ErrorErrorCat);
@@ -99,4 +100,14 @@ std::error_code StringError::convertToErrorCode() const {
   return EC;
 }
 
+void report_fatal_error(Error Err, bool GenCrashDiag) {
+  assert(Err && "report_fatal_error called with success value");
+  std::string ErrMsg;
+  {
+    raw_string_ostream ErrStream(ErrMsg);
+    logAllUnhandledErrors(std::move(Err), ErrStream, "");
+  }
+  report_fatal_error(ErrMsg);
+}
+
 }

llvm/tools/dsymutil/BinaryHolder.cpp

@@ -102,10 +102,8 @@ BinaryHolder::GetArchiveMemberBuffers(StringRef Filename,
   Buffers.reserve(CurrentArchives.size());
 
   for (const auto &CurrentArchive : CurrentArchives) {
-    for (auto ChildOrErr : CurrentArchive->children()) {
-      if (std::error_code Err = ChildOrErr.getError())
-        return Err;
-      const auto &Child = *ChildOrErr;
+    Error Err;
+    for (auto Child : CurrentArchive->children(Err)) {
       if (auto NameOrErr = Child.getName()) {
         if (*NameOrErr == Filename) {
           if (Timestamp != sys::TimeValue::PosixZeroTime() &&
@@ -123,6 +121,8 @@ BinaryHolder::GetArchiveMemberBuffers(StringRef Filename,
         }
       }
     }
+    if (Err)
+      return errorToErrorCode(std::move(Err));
   }
 
   if (Buffers.empty())

llvm/tools/llvm-ar/llvm-ar.cpp

@@ -405,35 +405,37 @@ static void performReadOperation(ArchiveOperation Operation,
     fail("extracting from a thin archive is not supported");
 
   bool Filter = !Members.empty();
-  for (auto &ChildOrErr : OldArchive->children()) {
-    failIfError(ChildOrErr.getError());
-    const object::Archive::Child &C = *ChildOrErr;
-
-    ErrorOr<StringRef> NameOrErr = C.getName();
-    failIfError(NameOrErr.getError());
-    StringRef Name = NameOrErr.get();
-
-    if (Filter) {
-      auto I = std::find(Members.begin(), Members.end(), Name);
-      if (I == Members.end())
-        continue;
-      Members.erase(I);
-    }
+  {
+    Error Err;
+    for (auto &C : OldArchive->children(Err)) {
+      ErrorOr<StringRef> NameOrErr = C.getName();
+      failIfError(NameOrErr.getError());
+      StringRef Name = NameOrErr.get();
 
-    switch (Operation) {
-    default:
-      llvm_unreachable("Not a read operation");
-    case Print:
-      doPrint(Name, C);
-      break;
-    case DisplayTable:
-      doDisplayTable(Name, C);
-      break;
-    case Extract:
-      doExtract(Name, C);
-      break;
+      if (Filter) {
+        auto I = std::find(Members.begin(), Members.end(), Name);
+        if (I == Members.end())
+          continue;
+        Members.erase(I);
+      }
+
+      switch (Operation) {
+      default:
+        llvm_unreachable("Not a read operation");
+      case Print:
+        doPrint(Name, C);
+        break;
+      case DisplayTable:
+        doDisplayTable(Name, C);
+        break;
+      case Extract:
+        doExtract(Name, C);
+        break;
+      }
     }
+    failIfError(std::move(Err));
   }
+
   if (Members.empty())
     return;
   for (StringRef Name : Members)
@@ -531,9 +533,8 @@ computeNewArchiveMembers(ArchiveOperation Operation,
   int InsertPos = -1;
   StringRef PosName = sys::path::filename(RelPos);
   if (OldArchive) {
-    for (auto &ChildOrErr : OldArchive->children()) {
-      failIfError(ChildOrErr.getError());
-      auto &Child = ChildOrErr.get();
+    Error Err;
+    for (auto &Child : OldArchive->children(Err)) {
       int Pos = Ret.size();
       ErrorOr<StringRef> NameOrErr = Child.getName();
       failIfError(NameOrErr.getError());
@@ -568,6 +569,7 @@ computeNewArchiveMembers(ArchiveOperation Operation,
       if (MemberI != Members.end())
         Members.erase(MemberI);
     }
+    failIfError(std::move(Err));
   }
 
   if (Operation == Delete)
@@ -764,9 +766,11 @@ static void runMRIScript() {
                   "Could not parse library");
       Archives.push_back(std::move(*LibOrErr));
       object::Archive &Lib = *Archives.back();
-      for (auto &MemberOrErr : Lib.children()) {
-        failIfError(MemberOrErr.getError());
-        addMember(NewMembers, *MemberOrErr);
+      {
+        Error Err;
+        for (auto &Member : Lib.children(Err))
+          addMember(NewMembers, Member);
+        failIfError(std::move(Err));
       }
       break;
     }