Next step along the way to getting good error messages for bad archives.

This step builds on Lang Hames work to change Archive::child_iterator
for better interoperation with Error/Expected.  Building on that it is now
possible to return an error message when the size field of an archive
contains non-decimal characters.

llvm-svn: 276025

5 files changed, 77 insertions, 58 deletions

diff --git a/llvm/include/llvm/Object/Archive.h b/llvm/include/llvm/Object/Archive.h
index cfba2567371..c71f138c67e 100644
--- a/llvm/include/llvm/Object/Archive.h
+++ b/llvm/include/llvm/Object/Archive.h
@@ -38,7 +38,7 @@ struct ArchiveMemberHeader {
   llvm::StringRef getName() const;
 
   /// Members are not larger than 4GB.
-  ErrorOr<uint32_t> getSize() const;
+  Expected<uint32_t> getSize() const;
 
   sys::fs::perms getAccessMode() const;
   sys::TimeValue getLastModified() const;
@@ -67,7 +67,7 @@ public:
     bool isThinMember() const;
 
   public:
-    Child(const Archive *Parent, const char *Start, std::error_code *EC);
+    Child(const Archive *Parent, const char *Start, Error *Err);
     Child(const Archive *Parent, StringRef Data, uint16_t StartOfFile);
 
     bool operator ==(const Child &other) const {
@@ -76,7 +76,7 @@ public:
     }
 
     const Archive *getParent() const { return Parent; }
-    ErrorOr<Child> getNext() const;
+    Expected<Child> getNext() const;
 
     ErrorOr<StringRef> getName() const;
     ErrorOr<std::string> getFullName() const;
@@ -93,9 +93,9 @@ public:
       return getHeader()->getAccessMode();
     }
     /// \return the size of the archive member without the header or padding.
-    ErrorOr<uint64_t> getSize() const;
+    Expected<uint64_t> getSize() const;
     /// \return the size in the archive header for this member.
-    ErrorOr<uint64_t> getRawSize() const;
+    Expected<uint64_t> getRawSize() const;
 
     ErrorOr<StringRef> getBuffer() const;
     uint64_t getChildOffset() const;
@@ -136,7 +136,7 @@ public:
       else {
         ErrorAsOutParameter ErrAsOutParam(*E);
         C = C.getParent()->child_end().C;
-        *E = errorCodeToError(ChildOrErr.getError());
+        *E = ChildOrErr.takeError();
         E = nullptr;
       }
       return *this;
diff --git a/llvm/lib/Object/Archive.cpp b/llvm/lib/Object/Archive.cpp
index daf301e2e7e..6d0330d52ec 100644
--- a/llvm/lib/Object/Archive.cpp
+++ b/llvm/lib/Object/Archive.cpp
@@ -27,6 +27,13 @@ static const char *const ThinMagic = "!<thin>\n";
 
 void Archive::anchor() { }
 
+static Error
+malformedError(Twine Msg) {
+  std::string StringMsg = "truncated or malformed archive (" + Msg.str() + ")";
+  return make_error<GenericBinaryError>(std::move(StringMsg),
+                                        object_error::parse_failed);
+}
+
 StringRef ArchiveMemberHeader::getName() const {
   char EndCond;
   if (Name[0] == '/' || Name[0] == '#')
@@ -42,10 +49,16 @@ StringRef ArchiveMemberHeader::getName() const {
   return llvm::StringRef(Name, end);
 }
 
-ErrorOr<uint32_t> ArchiveMemberHeader::getSize() const {
+Expected<uint32_t> ArchiveMemberHeader::getSize() const {
   uint32_t Ret;
-  if (llvm::StringRef(Size, sizeof(Size)).rtrim(" ").getAsInteger(10, Ret))
-    return object_error::parse_failed; // Size is not a decimal number.
+  if (llvm::StringRef(Size, sizeof(Size)).rtrim(" ").getAsInteger(10, Ret)) {
+    std::string Buf;
+    raw_string_ostream OS(Buf);
+    OS.write_escaped(llvm::StringRef(Size, sizeof(Size)).rtrim(" "));
+    OS.flush();
+    return malformedError("characters in size field in archive header are not "
+                          "all decimal numbers: '" + Buf + "'");
+  }
   return Ret;
 }
 
@@ -91,8 +104,7 @@ Archive::Child::Child(const Archive *Parent, StringRef Data,
                       uint16_t StartOfFile)
     : Parent(Parent), Data(Data), StartOfFile(StartOfFile) {}
 
-Archive::Child::Child(const Archive *Parent, const char *Start,
-                      std::error_code *EC)
+Archive::Child::Child(const Archive *Parent, const char *Start, Error *Err)
     : Parent(Parent) {
   if (!Start)
     return;
@@ -100,9 +112,14 @@ Archive::Child::Child(const Archive *Parent, const char *Start,
   uint64_t Size = sizeof(ArchiveMemberHeader);
   Data = StringRef(Start, Size);
   if (!isThinMember()) {
-    ErrorOr<uint64_t> MemberSize = getRawSize();
-    if ((*EC = MemberSize.getError()))
+    Expected<uint64_t> MemberSize = getRawSize();
+    if (!MemberSize) {
+      if (Err) {
+        ErrorAsOutParameter ErrAsOutParam(*Err);
+        *Err = MemberSize.takeError();
+      }
       return;
+    }
     Size += MemberSize.get();
     Data = StringRef(Start, Size);
   }
@@ -119,21 +136,18 @@ Archive::Child::Child(const Archive *Parent, const char *Start,
   }
 }
 
-ErrorOr<uint64_t> Archive::Child::getSize() const {
+Expected<uint64_t> Archive::Child::getSize() const {
   if (Parent->IsThin) {
-    ErrorOr<uint32_t> Size = getHeader()->getSize();
-    if (std::error_code EC = Size.getError())
-      return EC;
+    Expected<uint32_t> Size = getHeader()->getSize();
+    if (!Size)
+      return Size.takeError();
     return Size.get();
   }
   return Data.size() - StartOfFile;
 }
 
-ErrorOr<uint64_t> Archive::Child::getRawSize() const {
-  ErrorOr<uint32_t> Size = getHeader()->getSize();
-  if (std::error_code EC = Size.getError())
-    return EC;
-  return Size.get();
+Expected<uint64_t> Archive::Child::getRawSize() const {
+  return getHeader()->getSize();
 }
 
 bool Archive::Child::isThinMember() const {
@@ -158,9 +172,9 @@ ErrorOr<std::string> Archive::Child::getFullName() const {
 
 ErrorOr<StringRef> Archive::Child::getBuffer() const {
   if (!isThinMember()) {
-    ErrorOr<uint32_t> Size = getSize();
-    if (std::error_code EC = Size.getError())
-      return EC;
+    Expected<uint32_t> Size = getSize();
+    if (!Size)
+      return errorToErrorCode(Size.takeError());
     return StringRef(Data.data() + StartOfFile, Size.get());
   }
   ErrorOr<std::string> FullNameOrEr = getFullName();
@@ -174,7 +188,7 @@ ErrorOr<StringRef> Archive::Child::getBuffer() const {
   return Parent->ThinBuffers.back()->getBuffer();
 }
 
-ErrorOr<Archive::Child> Archive::Child::getNext() const {
+Expected<Archive::Child> Archive::Child::getNext() const {
   size_t SpaceToSkip = Data.size();
   // If it's odd, add 1 to make it even.
   if (SpaceToSkip & 1)
@@ -188,12 +202,13 @@ ErrorOr<Archive::Child> Archive::Child::getNext() const {
 
   // Check to see if this is past the end of the archive.
   if (NextLoc > Parent->Data.getBufferEnd())
-    return object_error::parse_failed;
+    return malformedError("offset to next archive member past the end of the "
+                          "archive");
 
-  std::error_code EC;
-  Child Ret(Parent, NextLoc, &EC);
-  if (EC)
-    return EC;
+  Error Err;
+  Child Ret(Parent, NextLoc, &Err);
+  if (Err)
+    return std::move(Err);
   return Ret;
 }
 
@@ -472,13 +487,9 @@ Archive::child_iterator Archive::child_begin(Error &Err,
                           &Err);
 
   const char *Loc = Data.getBufferStart() + strlen(Magic);
-  std::error_code EC;
-  Child C(this, Loc, &EC);
-  if (EC) {
-    ErrorAsOutParameter ErrAsOutParam(Err);
-    Err = errorCodeToError(EC);
+  Child C(this, Loc, &Err);
+  if (Err)
     return child_end();
-  }
   return child_iterator(C, &Err);
 }
 
@@ -543,10 +554,10 @@ ErrorOr<Archive::Child> Archive::Symbol::getMember() const {
   }
 
   const char *Loc = Parent->getData().begin() + Offset;
-  std::error_code EC;
-  Child C(Parent, Loc, &EC);
-  if (EC)
-    return EC;
+  Error Err;
+  Child C(Parent, Loc, &Err);
+  if (Err)
+    return errorToErrorCode(std::move(Err));
   return C;
 }
 
diff --git a/llvm/test/tools/llvm-objdump/malformed-archives.test b/llvm/test/tools/llvm-objdump/malformed-archives.test
index c477022e2e1..b44b1609d60 100644
--- a/llvm/test/tools/llvm-objdump/malformed-archives.test
+++ b/llvm/test/tools/llvm-objdump/malformed-archives.test
@@ -5,16 +5,16 @@
 # RUN:   %p/Inputs/libbogus1.a \
 # RUN:   2>&1 | FileCheck -check-prefix=bogus1 %s 
 
-# bogus1: Invalid data was encountered while parsing the file
+# bogus1: libbogus1.a': truncated or malformed archive (characters in size field in archive header are not all decimal numbers: '10%')
 
 # RUN: not llvm-objdump -macho -archive-headers \
 # RUN:   %p/Inputs/libbogus2.a \
 # RUN:   2>&1 | FileCheck -check-prefix=bogus2 %s 
 
-# bogus2: LLVM ERROR: Invalid data was encountered while parsing the file
+# bogus2: libbogus2.a': truncated or malformed archive (characters in size field in archive header are not all decimal numbers: '1%')
 
 # RUN: not llvm-objdump -macho -archive-headers \
 # RUN:   %p/Inputs/libbogus3.a \
 # RUN:   2>&1 | FileCheck -check-prefix=bogus3 %s 
 
-# bogus3: LLVM ERROR: Invalid data was encountered while parsing the file
+# bogus3: libbogus3.a': truncated or malformed archive (offset to next archive member past the end of the archive)
diff --git a/llvm/tools/llvm-ar/llvm-ar.cpp b/llvm/tools/llvm-ar/llvm-ar.cpp
index 865152b6af8..11dd7ea7c0f 100644
--- a/llvm/tools/llvm-ar/llvm-ar.cpp
+++ b/llvm/tools/llvm-ar/llvm-ar.cpp
@@ -343,8 +343,8 @@ static void doDisplayTable(StringRef Name, const object::Archive::Child &C) {
     printMode(Mode & 007);
     outs() << ' ' << C.getUID();
     outs() << '/' << C.getGID();
-    ErrorOr<uint64_t> Size = C.getSize();
-    failIfError(Size.getError());
+    Expected<uint64_t> Size = C.getSize();
+    failIfError(Size.takeError());
     outs() << ' ' << format("%6llu", Size.get());
     outs() << ' ' << C.getLastModified().str();
     outs() << ' ';
diff --git a/llvm/tools/llvm-objdump/MachODump.cpp b/llvm/tools/llvm-objdump/MachODump.cpp
index 4d950f1d7bd..10cd407e377 100644
--- a/llvm/tools/llvm-objdump/MachODump.cpp
+++ b/llvm/tools/llvm-objdump/MachODump.cpp
@@ -1472,8 +1472,9 @@ static void printMachOUniversalHeaders(const object::MachOUniversalBinary *UB,
   }
 }
 
-static void printArchiveChild(const Archive::Child &C, bool verbose,
-                              bool print_offset) {
+static void printArchiveChild(StringRef Filename, const Archive::Child &C,
+                              bool verbose, bool print_offset,
+                              StringRef ArchitectureName = StringRef()) {
   if (print_offset)
     outs() << C.getChildOffset() << "\t";
   sys::fs::perms Mode = C.getAccessMode();
@@ -1498,9 +1499,9 @@ static void printArchiveChild(const Archive::Child &C, bool verbose,
   outs() << format("%3d/", UID);
   unsigned GID = C.getGID();
   outs() << format("%-3d ", GID);
-  ErrorOr<uint64_t> Size = C.getRawSize();
-  if (std::error_code EC = Size.getError())
-    report_fatal_error(EC.message());
+  Expected<uint64_t> Size = C.getRawSize();
+  if (!Size)
+    report_error(Filename, C, Size.takeError(), ArchitectureName);
   outs() << format("%5" PRId64, Size.get()) << " ";
 
   StringRef RawLastModified = C.getRawLastModified();
@@ -1534,12 +1535,15 @@ static void printArchiveChild(const Archive::Child &C, bool verbose,
   }
 }
 
-static void printArchiveHeaders(Archive *A, bool verbose, bool print_offset) {
+static void printArchiveHeaders(StringRef Filename, Archive *A, bool verbose,
+                                bool print_offset,
+                                StringRef ArchitectureName = StringRef()) {
   Error Err;
   for (const auto &C : A->children(Err, false))
-    printArchiveChild(C, verbose, print_offset);
+    printArchiveChild(Filename, C, verbose, print_offset, ArchitectureName);
+
   if (Err)
-    report_fatal_error(std::move(Err));
+    report_error(Filename, std::move(Err));
 }
 
 // ParseInputMachO() parses the named Mach-O file in Filename and handles the
@@ -1569,7 +1573,8 @@ void llvm::ParseInputMachO(StringRef Filename) {
   if (Archive *A = dyn_cast<Archive>(&Bin)) {
     outs() << "Archive : " << Filename << "\n";
     if (ArchiveHeaders)
-      printArchiveHeaders(A, !NonVerbose, ArchiveMemberOffsets);
+      printArchiveHeaders(Filename, A, !NonVerbose, ArchiveMemberOffsets);
+
     Error Err;
     for (auto &C : A->children(Err)) {
       Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
@@ -1626,7 +1631,8 @@ void llvm::ParseInputMachO(StringRef Filename) {
                 outs() << " (architecture " << ArchitectureName << ")";
               outs() << "\n";
               if (ArchiveHeaders)
-                printArchiveHeaders(A.get(), !NonVerbose, ArchiveMemberOffsets);
+                printArchiveHeaders(Filename, A.get(), !NonVerbose,
+                                    ArchiveMemberOffsets, ArchitectureName);
               Error Err;
               for (auto &C : A->children(Err)) {
                 Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
@@ -1681,7 +1687,8 @@ void llvm::ParseInputMachO(StringRef Filename) {
             std::unique_ptr<Archive> &A = *AOrErr;
             outs() << "Archive : " << Filename << "\n";
             if (ArchiveHeaders)
-              printArchiveHeaders(A.get(), !NonVerbose, ArchiveMemberOffsets);
+              printArchiveHeaders(Filename, A.get(), !NonVerbose,
+                                  ArchiveMemberOffsets);
             Error Err;
             for (auto &C : A->children(Err)) {
               Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
@@ -1732,7 +1739,8 @@ void llvm::ParseInputMachO(StringRef Filename) {
           outs() << " (architecture " << ArchitectureName << ")";
         outs() << "\n";
         if (ArchiveHeaders)
-          printArchiveHeaders(A.get(), !NonVerbose, ArchiveMemberOffsets);
+          printArchiveHeaders(Filename, A.get(), !NonVerbose,
+                              ArchiveMemberOffsets, ArchitectureName);
         Error Err;
         for (auto &C : A->children(Err)) {
           Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();