[llvm-objdump] - Remove one of `report_error` functions and improve the error reporting.

[grimar]

Summary

One of the report_error functions was taking object::Archive::Child as an
argument. It feels excessive, this patch removes it and introduce a helper
function instead. Also I fixed a "TODO" in this patch what improved the message printed.

[jhenderson]

tools/llvm-objdump/MachODump.cpp

2216 ↗

(On Diff #216097)

Ndx -> ChildIndex

tools/llvm-objdump/llvm-objdump.cpp

372 ↗

(On Diff #216097)

Comment to explain why we're consuming the error?

441 ↗

(On Diff #216097)

Did you mean to delete this function?

[grimar]

• Addressed review comments.

tools/llvm-objdump/llvm-objdump.cpp

441 ↗

(On Diff #216097)

Yes..

[grimar]

• Ndx->ChildIndex in 2 more missed places.

[MaskRay]

tools/llvm-objdump/MachODump.cpp

2486 ↗

(On Diff #216100)

Missing ++I ?

[jhenderson]

tools/llvm-objdump/llvm-objdump.cpp

372 ↗

(On Diff #216105)

a error -> an error

print the index of which archive member this is -> print the index of the archive member

373 ↗

(On Diff #216105)

we already -> we are already

[grimar]

• Addressed review comments.

tools/llvm-objdump/MachODump.cpp

2486 ↗

(On Diff #216100)

Yes :/ thanks.

[jhenderson]

LGTM, with one nit.

tools/llvm-objdump/llvm-objdump.cpp

373 ↗

(On Diff #216112)

a error -> an error

llvm/trunk/test/tools/llvm-objdump/malformed-archives.test

 # RUN:   2>&1 | FileCheck -check-prefix=bogus7 %s 
 
 # bogus7: libbogus7.a': truncated or malformed archive (long name length characters after the #1/ are not all decimal numbers: '@123$' for archive member header at offset 8)
 
 # RUN: not llvm-objdump -macho -archive-headers \
 # RUN:   %p/Inputs/libbogus8.a \
 # RUN:   2>&1 | FileCheck -check-prefix=bogus8 %s 
 
-# bogus8: libbogus8.a(???): truncated or malformed archive (long name length: 1234 extends past the end of the member or archive for archive member header at offset 86)
+# bogus8: libbogus8.a(<file index: 1>): truncated or malformed archive (long name length: 1234 extends past the end of the member or archive for archive member header at offset 86)
 
 # RUN: not llvm-objdump -s %p/Inputs/libbogus9.a \
 # RUN:   2>&1 | FileCheck -check-prefix=bogus9 %s 
 
-# bogus9: libbogus9.a(???): truncated or malformed archive (long name offset characters after the '/' are not all decimal numbers: '&a25*' for archive member header at offset 94)
+# bogus9: libbogus9.a(<file index: 0>): truncated or malformed archive (long name offset characters after the '/' are not all decimal numbers: '&a25*' for archive member header at offset 94)
 
 # RUN: not llvm-objdump -s %p/Inputs/libbogus10.a \
 # RUN:   2>&1 | FileCheck -check-prefix=bogus10 %s 
 
-# bogus10: libbogus10.a(???): truncated or malformed archive (long name offset 507 past the end of the string table for archive member header at offset 94)
+# bogus10: libbogus10.a(<file index: 0>): truncated or malformed archive (long name offset 507 past the end of the string table for archive member header at offset 94)
 
 # RUN: not llvm-objdump -macho -archive-headers \
 # RUN:   %p/Inputs/libbogus11.a \
 # RUN:   2>&1 | FileCheck -check-prefix=bogus11 %s 
 
 # bogus11: libbogus11.a(hello.c): truncated or malformed archive (characters in UID field in archive header are not all decimal numbers: '~97&' for the archive member header at offset 8)
 
 # RUN: not llvm-objdump -macho -archive-headers \

llvm/trunk/tools/llvm-objdump/MachODump.cpp

if (big_size > size)
       outs() << " (past end of file)";
     outs() << "\n";
     outs() << "    align 2^" << OFA.getAlign() << " (" << (1 << OFA.getAlign())
            << ")\n";
   }
 }
 
 static void printArchiveChild(StringRef Filename, const Archive::Child &C,
-                              bool verbose, bool print_offset,
+                              size_t ChildIndex, bool verbose,
+                              bool print_offset,
                               StringRef ArchitectureName = StringRef()) {
   if (print_offset)
     outs() << C.getChildOffset() << "\t";
   sys::fs::perms Mode =
-      unwrapOrError(C.getAccessMode(), Filename, C, ArchitectureName);
+      unwrapOrError(C.getAccessMode(), Filename,
+                    getFileNameForError(C, ChildIndex),
+                    ArchitectureName);
   if (verbose) {
     // FIXME: this first dash, "-", is for (Mode & S_IFMT) == S_IFREG.
     // But there is nothing in sys::fs::perms for S_IFMT or S_IFREG.
     outs() << "-";
     outs() << ((Mode & sys::fs::owner_read) ? "r" : "-");
     outs() << ((Mode & sys::fs::owner_write) ? "w" : "-");
     outs() << ((Mode & sys::fs::owner_exe) ? "x" : "-");
     outs() << ((Mode & sys::fs::group_read) ? "r" : "-");
     outs() << ((Mode & sys::fs::group_write) ? "w" : "-");
     outs() << ((Mode & sys::fs::group_exe) ? "x" : "-");
     outs() << ((Mode & sys::fs::others_read) ? "r" : "-");
     outs() << ((Mode & sys::fs::others_write) ? "w" : "-");
     outs() << ((Mode & sys::fs::others_exe) ? "x" : "-");
   } else {
     outs() << format("0%o ", Mode);
   }
 
   outs() << format(
       "%3d/%-3d %5" PRId64 " ",
-      unwrapOrError(C.getUID(), Filename, C, ArchitectureName),
-      unwrapOrError(C.getGID(), Filename, C, ArchitectureName),
-      unwrapOrError(C.getRawSize(), Filename, C, ArchitectureName));
+      unwrapOrError(C.getUID(), Filename, getFileNameForError(C, ChildIndex),
+                    ArchitectureName),
+      unwrapOrError(C.getGID(), Filename, getFileNameForError(C, ChildIndex),
+                    ArchitectureName),
+      unwrapOrError(C.getRawSize(), Filename,
+                    getFileNameForError(C, ChildIndex), ArchitectureName));
 
   StringRef RawLastModified = C.getRawLastModified();
   if (verbose) {
     unsigned Seconds;
     if (RawLastModified.getAsInteger(10, Seconds))
       outs() << "(date: \"" << RawLastModified
              << "\" contains non-decimal chars) ";
     else {
       // Since cime(3) returns a 26 character string of the form:
       // "Sun Sep 16 01:03:52 1973\n\0"
       // just print 24 characters.
       time_t t = Seconds;
       outs() << format("%.24s ", ctime(&t));
     }
   } else {
     outs() << RawLastModified << " ";
   }
 
   if (verbose) {
     Expected<StringRef> NameOrErr = C.getName();
     if (!NameOrErr) {
       consumeError(NameOrErr.takeError());
-      outs() << unwrapOrError(C.getRawName(), Filename, C, ArchitectureName)
+      outs() << unwrapOrError(C.getRawName(), Filename,
+                              getFileNameForError(C, ChildIndex),
+                              ArchitectureName)
              << "\n";
     } else {
       StringRef Name = NameOrErr.get();
       outs() << Name << "\n";
     }
   } else {
-    outs() << unwrapOrError(C.getRawName(), Filename, C, ArchitectureName)
+    outs() << unwrapOrError(C.getRawName(), Filename,
+                            getFileNameForError(C, ChildIndex),
+                            ArchitectureName)
            << "\n";
   }
 }
 
 static void printArchiveHeaders(StringRef Filename, Archive *A, bool verbose,
                                 bool print_offset,
                                 StringRef ArchitectureName = StringRef()) {
   Error Err = Error::success();
+  size_t I = 0;
   for (const auto &C : A->children(Err, false))
-    printArchiveChild(Filename, C, verbose, print_offset, ArchitectureName);
+    printArchiveChild(Filename, C, I++, verbose, print_offset,
+                      ArchitectureName);
 
   if (Err)
     report_error(std::move(Err), StringRef(), Filename, ArchitectureName);
 }
 
 static bool ValidateArchFlags() {
   // Check for -arch all and verifiy the -arch flags are valid.
   for (unsigned i = 0; i < ArchFlags.size(); ++i) {
void parseInputMachO(StringRef Filename) {
   Binary &Bin = *BinaryOrErr.get().getBinary();
 
   if (Archive *A = dyn_cast<Archive>(&Bin)) {
     outs() << "Archive : " << Filename << "\n";
     if (ArchiveHeaders)
       printArchiveHeaders(Filename, A, !NonVerbose, ArchiveMemberOffsets);
 
     Error Err = Error::success();
+    unsigned I = -1;
     for (auto &C : A->children(Err)) {
+      ++I;
       Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
       if (!ChildOrErr) {
         if (Error E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
-          report_error(std::move(E), Filename, C);
+          report_error(std::move(E), Filename, getFileNameForError(C, I));
         continue;
       }
       if (MachOObjectFile *O = dyn_cast<MachOObjectFile>(&*ChildOrErr.get())) {
         if (!checkMachOAndArchFlags(O, Filename))
           return;
         ProcessMachO(Filename, O, O->getFileName());
       }
     }
for (unsigned i = 0; i < ArchFlags.size(); ++i) {
             outs() << "Archive : " << Filename;
             if (!ArchitectureName.empty())
               outs() << " (architecture " << ArchitectureName << ")";
             outs() << "\n";
             if (ArchiveHeaders)
               printArchiveHeaders(Filename, A.get(), !NonVerbose,
                                   ArchiveMemberOffsets, ArchitectureName);
             Error Err = Error::success();
+            unsigned I = -1;
             for (auto &C : A->children(Err)) {
+              ++I;
               Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
               if (!ChildOrErr) {
-                if (Error E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
-                  report_error(std::move(E), Filename, C, ArchitectureName);
+                if (Error E =
+                        isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
+                  report_error(std::move(E), Filename, getFileNameForError(C, I),
+                               ArchitectureName);
                 continue;
               }
               if (MachOObjectFile *O =
                       dyn_cast<MachOObjectFile>(&*ChildOrErr.get()))
                 ProcessMachO(Filename, O, O->getFileName(), ArchitectureName);
             }
             if (Err)
               report_error(std::move(Err), Filename);
for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
         } else if (Expected<std::unique_ptr<Archive>> AOrErr =
                        I->getAsArchive()) {
           std::unique_ptr<Archive> &A = *AOrErr;
           outs() << "Archive : " << Filename << "\n";
           if (ArchiveHeaders)
             printArchiveHeaders(Filename, A.get(), !NonVerbose,
                                 ArchiveMemberOffsets);
           Error Err = Error::success();
+          unsigned I = -1;
           for (auto &C : A->children(Err)) {
+            ++I;
             Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
             if (!ChildOrErr) {
               if (Error E =
                       isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
-                report_error(std::move(E), Filename, C);
+                report_error(std::move(E), Filename, getFileNameForError(C, I));
               continue;
             }
             if (MachOObjectFile *O =
                     dyn_cast<MachOObjectFile>(&*ChildOrErr.get()))
               ProcessMachO(Filename, O, O->getFileName());
           }
           if (Err)
             report_error(std::move(Err), Filename);
if (ObjOrErr) {
       outs() << "Archive : " << Filename;
       if (!ArchitectureName.empty())
         outs() << " (architecture " << ArchitectureName << ")";
       outs() << "\n";
       if (ArchiveHeaders)
         printArchiveHeaders(Filename, A.get(), !NonVerbose,
                             ArchiveMemberOffsets, ArchitectureName);
       Error Err = Error::success();
+      unsigned I = -1;
       for (auto &C : A->children(Err)) {
+        ++I;
         Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
         if (!ChildOrErr) {
           if (Error E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
-            report_error(std::move(E), Filename, C, ArchitectureName);
+            report_error(std::move(E), Filename, getFileNameForError(C, I),
+                         ArchitectureName);
           continue;
         }
         if (MachOObjectFile *O =
                 dyn_cast<MachOObjectFile>(&*ChildOrErr.get())) {
           if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(O))
             ProcessMachO(Filename, MachOOF, MachOOF->getFileName(),
                           ArchitectureName);
         }

llvm/trunk/tools/llvm-objdump/llvm-objdump.h

void printSymbolTable(const object::ObjectFile *O, StringRef ArchiveName,
                       StringRef ArchitectureName = StringRef());
 void warn(Twine Message);
 LLVM_ATTRIBUTE_NORETURN void error(Twine Message);
 LLVM_ATTRIBUTE_NORETURN void report_error(StringRef File, Twine Message);
 LLVM_ATTRIBUTE_NORETURN void report_error(Error E, StringRef File);
 LLVM_ATTRIBUTE_NORETURN void
 report_error(Error E, StringRef FileName, StringRef ArchiveName,
              StringRef ArchitectureName = StringRef());
-LLVM_ATTRIBUTE_NORETURN void
-report_error(Error E, StringRef ArchiveName, const object::Archive::Child &C,
-             StringRef ArchitectureName = StringRef());
 
 template <typename T, typename... Ts>
 T unwrapOrError(Expected<T> EO, Ts &&... Args) {
   if (EO)
     return std::move(*EO);
   report_error(EO.takeError(), std::forward<Ts>(Args)...);
 }
 
+std::string getFileNameForError(const object::Archive::Child &C,
+                                unsigned Index);
+
 } // end namespace llvm
 
 #endif

llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp

if (!SecName.empty())
     FoundSectionSet.insert(SecName);
   return is_contained(FilterSections, SecName);
 }
 
 SectionFilter ToolSectionFilter(object::ObjectFile const &O) {
   return SectionFilter([](object::SectionRef S) { return shouldKeep(S); }, O);
 }
 
+std::string getFileNameForError(const object::Archive::Child &C,
+                                unsigned Index) {
+  Expected<StringRef> NameOrErr = C.getName();
+  if (NameOrErr)
+    return NameOrErr.get();
+  // If we have an error getting the name then we print the index of the archive
+  // member. Since we are already in an error state, we just ignore this error.
+  consumeError(NameOrErr.takeError());
+  return "<file index: " + std::to_string(Index) + ">";
+}
+
 void error(std::error_code EC) {
   if (!EC)
     return;
   WithColor::error(errs(), ToolName)
       << "reading file: " << EC.message() << ".\n";
   errs().flush();
   exit(1);
 }
LLVM_ATTRIBUTE_NORETURN void report_error(Error E, StringRef ArchiveName,
   std::string Buf;
   raw_string_ostream OS(Buf);
   logAllUnhandledErrors(std::move(E), OS);
   OS.flush();
   errs() << ": " << Buf;
   exit(1);
 }
 
-LLVM_ATTRIBUTE_NORETURN void report_error(Error E, StringRef ArchiveName,
-                                          const object::Archive::Child &C,
-                                          StringRef ArchitectureName) {
-  Expected<StringRef> NameOrErr = C.getName();
-  // TODO: if we have a error getting the name then it would be nice to print
-  // the index of which archive member this is and or its offset in the
-  // archive instead of "???" as the name.
-  if (!NameOrErr) {
-    consumeError(NameOrErr.takeError());
-    report_error(std::move(E), ArchiveName, "???", ArchitectureName);
-  } else
-    report_error(std::move(E), ArchiveName, NameOrErr.get(), ArchitectureName);
-}
-
 static void warnOnNoMatchForSections() {
   SetVector<StringRef> MissingSections;
   for (StringRef S : FilterSections) {
     if (FoundSectionSet.count(S))
       return;
     // User may specify a unnamed section. Don't warn for it.
     if (!S.empty())
       MissingSections.insert(S);
if (ArchiveHeaders && !MachOOpt && C)
     printArchiveChild(ArchiveName, *C);
   if (SymbolTable)
     printCOFFSymbolTable(I);
 }
 
 /// Dump each object file in \a a;
 static void dumpArchive(const Archive *A) {
   Error Err = Error::success();
+  unsigned I = -1;
   for (auto &C : A->children(Err)) {
+    ++I;
     Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
     if (!ChildOrErr) {
       if (auto E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
-        report_error(std::move(E), A->getFileName(), C);
+        report_error(std::move(E), A->getFileName(), getFileNameForError(C, I));
       continue;
     }
     if (ObjectFile *O = dyn_cast<ObjectFile>(&*ChildOrErr.get()))
       dumpObject(O, A, &C);
     else if (COFFImportFile *I = dyn_cast<COFFImportFile>(&*ChildOrErr.get()))
       dumpObject(I, A, &C);
     else
       report_error(errorCodeToError(object_error::invalid_file_type),