This removes the eating of the error in Archive::Child::getSize() when the characters

in the size field in the archive header for the member is not a number.  To do this we
have all of the needed methods return ErrorOr to push them up until we get out of lib.
Then the tools and can handle the error in whatever way is appropriate for that tool.

So the solution is to plumb all the ErrorOr stuff through everything that touches archives.
This include its iterators as one can create an Archive object but the first or any other
Child object may fail to be created due to a bad size field in its header.

Thanks to Lang Hames on the changes making child_iterator contain an
ErrorOr<Child> instead of a Child and the needed changes to ErrorOr.h to add
operator overloading for * and -> .

We don’t want to use llvm_unreachable() as it calls abort() and is produces a “crash”
and using report_fatal_error() to move the error checking will cause the program to
stop, neither of which are really correct in library code. There are still some uses of
these that should be cleaned up in this library code for other than the size field.

Also corrected the code where the size gets us to the “at the end of the archive”
which is OK but past the end of the archive will return object_error::parse_failed now.

The test cases use archives with text files so one can see the non-digit character,
in this case a ‘%’, in the size field.

llvm-svn: 250906

Author: enderby

llvm/include/llvm/Object/Archive.h

@@ -65,7 +65,10 @@ class Archive : public Binary {
     bool isThinMember() const;
 
   public:
-    Child(const Archive *Parent, const char *Start);
+    Child(const Archive *Parent, const char *Start,
+          std::error_code *EC);
+    static ErrorOr<std::unique_ptr<Child>> create(const Archive *Parent,
+                                                  const char *Start);
 
     bool operator ==(const Child &other) const {
       assert(Parent == other.Parent);
@@ -77,7 +80,7 @@ class Archive : public Binary {
     }
 
     const Archive *getParent() const { return Parent; }
-    Child getNext() const;
+    ErrorOr<Child> getNext() const;
 
     ErrorOr<StringRef> getName() const;
     StringRef getRawName() const { return getHeader()->getName(); }
@@ -93,9 +96,9 @@ class Archive : public Binary {
       return getHeader()->getAccessMode();
     }
     /// \return the size of the archive member without the header or padding.
-    uint64_t getSize() const;
+    ErrorOr<uint64_t> getSize() const;
     /// \return the size in the archive header for this member.
-    uint64_t getRawSize() const;
+    ErrorOr<uint64_t> getRawSize() const;
 
     ErrorOr<StringRef> getBuffer() const;
     uint64_t getChildOffset() const;
@@ -107,28 +110,35 @@ class Archive : public Binary {
   };
 
   class child_iterator {
-    Child child;
+    ErrorOr<Child> child;
 
   public:
-    child_iterator() : child(Child(nullptr, nullptr)) {}
+    child_iterator() : child(Child(nullptr, nullptr, nullptr)) {}
     child_iterator(const Child &c) : child(c) {}
-    const Child *operator->() const { return &child; }
-    const Child &operator*() const { return child; }
+    child_iterator(std::error_code EC) : child(EC) {}
+    const ErrorOr<Child> *operator->() const { return &child; }
+    const ErrorOr<Child> &operator*() const { return child; }
 
     bool operator==(const child_iterator &other) const {
-      return child == other.child;
+      if ((*this)->getError())
+        return false;
+      if (other->getError())
+        return false;
+      return (*this)->get() == other->get();
     }
 
     bool operator!=(const child_iterator &other) const {
       return !(*this == other);
     }
 
-    bool operator<(const child_iterator &other) const {
-      return child < other.child;
-    }
+    // No operator< as we can't do less than compares with iterators that
+    // contain errors.
 
+    // 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
-      child = child.getNext();
+      assert(child && "Can't increment iterator with error");
+      child = child->getNext();
       return *this;
     }
   };
@@ -212,7 +222,7 @@ class Archive : public Binary {
   StringRef getSymbolTable() const {
     // We know that the symbol table is not an external file,
     // so we just assert there is no error.
-    return *SymbolTable->getBuffer();
+    return *(*SymbolTable)->getBuffer();
   }
   uint32_t getNumberOfSymbols() const;
 

llvm/include/llvm/Support/ErrorOr.h

@@ -91,6 +91,7 @@ class ErrorOr {
   typedef typename std::remove_reference<T>::type &reference;
   typedef const typename std::remove_reference<T>::type &const_reference;
   typedef typename std::remove_reference<T>::type *pointer;
+  typedef const typename std::remove_reference<T>::type *const_pointer;
 
 public:
   template <class E>
@@ -183,10 +184,18 @@ class ErrorOr {
     return toPointer(getStorage());
   }
 
+  const_pointer operator ->() const {
+    return toPointer(getStorage());
+  }
+
   reference operator *() {
     return *getStorage();
   }
 
+  const_reference operator *() const {
+    return *getStorage();
+  }
+
 private:
   template <class OtherT>
   void copyConstruct(const ErrorOr<OtherT> &Other) {
@@ -246,10 +255,19 @@ class ErrorOr {
     return Val;
   }
 
+  const_pointer toPointer(const_pointer Val) const {
+    return Val;
+  }
+
   pointer toPointer(wrap *Val) {
     return &Val->get();
   }
 
+  const_pointer toPointer(const wrap *Val) const {
+    return &Val->get();
+  }
+
+
   storage_type *getStorage() {
     assert(!HasError && "Cannot get value when an error exists!");
     return reinterpret_cast<storage_type*>(TStorage.buffer);

llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp

@@ -318,10 +318,10 @@ RuntimeDyld::SymbolInfo MCJIT::findSymbol(const std::string &Name,
     object::Archive *A = OB.getBinary();
     // Look for our symbols in each Archive
     object::Archive::child_iterator ChildIt = A->findSym(Name);
-    if (ChildIt != A->child_end()) {
+    if (*ChildIt && ChildIt != A->child_end()) {
       // FIXME: Support nested archives?
       ErrorOr<std::unique_ptr<object::Binary>> ChildBinOrErr =
-          ChildIt->getAsBinary();
+        (*ChildIt)->getAsBinary();
       if (ChildBinOrErr.getError())
         continue;
       std::unique_ptr<object::Binary> &ChildBin = ChildBinOrErr.get();

llvm/lib/ExecutionEngine/Orc/OrcMCJITReplacement.h

@@ -253,10 +253,10 @@ class OrcMCJITReplacement : public ExecutionEngine {
       object::Archive *A = OB.getBinary();
       // Look for our symbols in each Archive
       object::Archive::child_iterator ChildIt = A->findSym(Name);
-      if (ChildIt != A->child_end()) {
+      if (*ChildIt && ChildIt != A->child_end()) {
         // FIXME: Support nested archives?
         ErrorOr<std::unique_ptr<object::Binary>> ChildBinOrErr =
-            ChildIt->getAsBinary();
+          (*ChildIt)->getAsBinary();
         if (ChildBinOrErr.getError())
           continue;
         std::unique_ptr<object::Binary> &ChildBin = ChildBinOrErr.get();