[llvm-objdump] - Remove one overload of reportError. NFCI.

[grimar]

Summary

There is a problem with reportError we have. Declaration says we have ArchiveName
that follows the FileName:

reportError(Error E, StringRef FileName, StringRef ArchiveName,...

Though implementation have them reversed. I cleaned it up and
removed an excessive reportError(Error E, StringRef File) version.

Rebased on top of D66418.

[grimar]

No test cases changed, so it is kind of NFCI.

[jhenderson]

tools/llvm-objdump/MachODump.cpp

2409 ↗

(On Diff #216346)

Should this be:

reportError(std::move(E), Filename, "", ArchitectureName);

[MaskRay]

Remove one of the reportError functions.

"Remove one overload of reportError" is conciser

[grimar]

tools/llvm-objdump/MachODump.cpp

2409 ↗

(On Diff #216346)

I am not familar with MachO, but I think - no. Seems MachOUniversalBinary can contain
multiple objects and also Filename is reported as archive name around this place.

Note that my change did not change the original logic, and also that this error is untested.

[MaskRay]

tools/llvm-objdump/MachODump.cpp

2409 ↗

(On Diff #216346)

I think George's change here is fine. At least it doesn't introduce new problems.

Maybe @seiya can help check whether there is something that should be improved? (@enderby seems to be inactive for a while)

[seiya]

tools/llvm-objdump/MachODump.cpp

2409 ↗

(On Diff #216346)

I prefer @jhenderson's suggestion. Reporting Filename as an archive name would look a bit weird since the filename is empty:

llvm-objdump: error: foo.fat.invalid(): ...

Note that users should be able to determine which object in the universal binary has a problem from its ArchitectureName (I believe there're no objects with the same cpu arch in an universal binary...).

[grimar]

tools/llvm-objdump/MachODump.cpp

2409 ↗

(On Diff #216346)

But my patch is NFC in current state. I'd like to keep it NFC.

Changing that can be done independently (before or after doing this change, and needs addind a test case then).

[seiya]

tools/llvm-objdump/MachODump.cpp

2409 ↗

(On Diff #216346)

Ah, I missed that. I agree with keeping this NFC.

[jhenderson]

LGTM.

tools/llvm-objdump/MachODump.cpp

2409 ↗

(On Diff #216346)

Okay, seems reasonable to me.

llvm/tools/llvm-objdump/MachODump.cpp

if (!ArchiveMemberName.empty()) {
     FileName = Name;
   }
 
   // If we need the symbol table to do the operation then check it here to
   // produce a good error message as to where the Mach-O file comes from in
   // the error message.
   if (Disassemble || IndirectSymbols || !FilterSections.empty() || UnwindInfo)
     if (Error Err = MachOOF->checkSymbolTable())
-      reportError(std::move(Err), ArchiveName, FileName, ArchitectureName);
+      reportError(std::move(Err), FileName, ArchiveName, ArchitectureName);
 
   if (DisassembleAll) {
     for (const SectionRef &Section : MachOOF->sections()) {
       StringRef SectName;
       if (Expected<StringRef> NameOrErr = Section.getName())
         SectName = *NameOrErr;
       else
         consumeError(NameOrErr.takeError());

 
 static void printArchiveChild(StringRef Filename, const Archive::Child &C,
                               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,
-                    getFileNameForError(C, ChildIndex),
-                    ArchitectureName);
+      unwrapOrError(C.getAccessMode(), getFileNameForError(C, ChildIndex),
+                    Filename, 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, getFileNameForError(C, ChildIndex),
-                    ArchitectureName),
-      unwrapOrError(C.getGID(), Filename, getFileNameForError(C, ChildIndex),
-                    ArchitectureName),
-      unwrapOrError(C.getRawSize(), Filename,
-                    getFileNameForError(C, ChildIndex), ArchitectureName));
+  outs() << format("%3d/%-3d %5" PRId64 " ",
+                   unwrapOrError(C.getUID(), getFileNameForError(C, ChildIndex),
+                                 Filename, ArchitectureName),
+                   unwrapOrError(C.getGID(), getFileNameForError(C, ChildIndex),
+                                 Filename, ArchitectureName),
+                   unwrapOrError(C.getRawSize(),
+                                 getFileNameForError(C, ChildIndex), Filename,
+                                 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,
-                              getFileNameForError(C, ChildIndex),
+      outs() << unwrapOrError(C.getRawName(),
+                              getFileNameForError(C, ChildIndex), Filename,
                               ArchitectureName)
              << "\n";
     } else {
       StringRef Name = NameOrErr.get();
       outs() << Name << "\n";
     }
   } else {
-    outs() << unwrapOrError(C.getRawName(), Filename,
-                            getFileNameForError(C, ChildIndex),
-                            ArchitectureName)
+    outs() << unwrapOrError(C.getRawName(), getFileNameForError(C, ChildIndex),
+                            Filename, 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, I++, verbose, print_offset,
                       ArchitectureName);
 
   if (Err)
-    reportError(std::move(Err), StringRef(), Filename, ArchitectureName);
+    reportError(std::move(Err), Filename, "", ArchitectureName);
 }
 
 static bool ValidateArchFlags() {
   // Check for -arch all and verifiy the -arch flags are valid.
   for (unsigned i = 0; i < ArchFlags.size(); ++i) {
     if (ArchFlags[i] == "all") {
       ArchAll = true;
     } else {
if (Archive *A = dyn_cast<Archive>(&Bin)) {
 
     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()))
-          reportError(std::move(E), Filename, getFileNameForError(C, I));
+          reportError(std::move(E), getFileNameForError(C, I), Filename);
         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) {
           if (ArchFlags.size() > 1)
             ArchitectureName = I->getArchFlagName();
           if (ObjOrErr) {
             ObjectFile &O = *ObjOrErr.get();
             if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(&O))
               ProcessMachO(Filename, MachOOF, "", ArchitectureName);
           } else if (Error E = isNotObjectErrorInvalidFileType(
                          ObjOrErr.takeError())) {
-            reportError(std::move(E), Filename, StringRef(), ArchitectureName);
+            reportError(std::move(E), "", Filename, ArchitectureName);
             continue;
           } else if (Expected<std::unique_ptr<Archive>> AOrErr =
                          I->getAsArchive()) {
             std::unique_ptr<Archive> &A = *AOrErr;
             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()))
-                  reportError(std::move(E), Filename, getFileNameForError(C, I),
+                  reportError(std::move(E), getFileNameForError(C, I), Filename,
                               ArchitectureName);
                 continue;
               }
               if (MachOObjectFile *O =
                       dyn_cast<MachOObjectFile>(&*ChildOrErr.get()))
                 ProcessMachO(Filename, O, O->getFileName(), ArchitectureName);
             }
             if (Err)
for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
           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()))
-                reportError(std::move(E), Filename, getFileNameForError(C, I));
+                reportError(std::move(E), getFileNameForError(C, I), Filename);
               continue;
             }
             if (MachOObjectFile *O =
                     dyn_cast<MachOObjectFile>(&*ChildOrErr.get()))
               ProcessMachO(Filename, O, O->getFileName());
           }
           if (Err)
             reportError(std::move(Err), Filename);
for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
     if (moreThanOneArch)
       ArchitectureName = I->getArchFlagName();
     if (ObjOrErr) {
       ObjectFile &Obj = *ObjOrErr.get();
       if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(&Obj))
         ProcessMachO(Filename, MachOOF, "", ArchitectureName);
     } else if (Error E =
                    isNotObjectErrorInvalidFileType(ObjOrErr.takeError())) {
-      reportError(std::move(E), StringRef(), Filename, ArchitectureName);
+      reportError(std::move(E), Filename, "", ArchitectureName);
     } else if (Expected<std::unique_ptr<Archive>> AOrErr = I->getAsArchive()) {
       std::unique_ptr<Archive> &A = *AOrErr;
       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()))
-            reportError(std::move(E), Filename, getFileNameForError(C, I),
+            reportError(std::move(E), getFileNameForError(C, I), Filename,
                         ArchitectureName);
           continue;
         }
         if (MachOObjectFile *O =
                 dyn_cast<MachOObjectFile>(&*ChildOrErr.get())) {
           if (MachOObjectFile *MachOOF = dyn_cast<MachOObjectFile>(O))
             ProcessMachO(Filename, MachOOF, MachOOF->getFileName(),
                           ArchitectureName);

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

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

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

void reportWarning(Twine Message, StringRef File) {
   errs().flush();
 }
 
 LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, Twine Message) {
   WithColor::error(errs(), ToolName) << "'" << File << "': " << Message << "\n";
   exit(1);
 }
 
-LLVM_ATTRIBUTE_NORETURN void reportError(Error E, StringRef File) {
-  assert(E);
-  std::string Buf;
-  raw_string_ostream OS(Buf);
-  logAllUnhandledErrors(std::move(E), OS);
-  OS.flush();
-  WithColor::error(errs(), ToolName) << "'" << File << "': " << Buf;
-  exit(1);
-}
-
-LLVM_ATTRIBUTE_NORETURN void reportError(Error E, StringRef ArchiveName,
-                                         StringRef FileName,
+LLVM_ATTRIBUTE_NORETURN void reportError(Error E, StringRef FileName,
+                                         StringRef ArchiveName,
                                          StringRef ArchitectureName) {
   assert(E);
   WithColor::error(errs(), ToolName);
   if (ArchiveName != "")
     errs() << ArchiveName << "(" << FileName << ")";
   else
     errs() << "'" << FileName << "'";
   if (!ArchitectureName.empty())
void printSymbolTable(const ObjectFile *O, StringRef ArchiveName,
   if (const COFFObjectFile *Coff = dyn_cast<const COFFObjectFile>(O)) {
     printCOFFSymbolTable(Coff);
     return;
   }
 
   const StringRef FileName = O->getFileName();
   for (auto I = O->symbol_begin(), E = O->symbol_end(); I != E; ++I) {
     const SymbolRef &Symbol = *I;
-    uint64_t Address = unwrapOrError(Symbol.getAddress(), ArchiveName, FileName,
+    uint64_t Address = unwrapOrError(Symbol.getAddress(), FileName, ArchiveName,
                                      ArchitectureName);
     if ((Address < StartAddress) || (Address > StopAddress))
       continue;
-    SymbolRef::Type Type = unwrapOrError(Symbol.getType(), ArchiveName,
-                                         FileName, ArchitectureName);
+    SymbolRef::Type Type = unwrapOrError(Symbol.getType(), FileName,
+                                         ArchiveName, ArchitectureName);
     uint32_t Flags = Symbol.getFlags();
-    section_iterator Section = unwrapOrError(Symbol.getSection(), ArchiveName,
-                                             FileName, ArchitectureName);
+    section_iterator Section = unwrapOrError(Symbol.getSection(), FileName,
+                                             ArchiveName, ArchitectureName);
     StringRef Name;
     if (Type == SymbolRef::ST_Debug && Section != O->section_end()) {
       if (Expected<StringRef> NameOrErr = Section->getName())
         Name = *NameOrErr;
       else
         consumeError(NameOrErr.takeError());
 
     } else {
-      Name = unwrapOrError(Symbol.getName(), ArchiveName, FileName,
+      Name = unwrapOrError(Symbol.getName(), FileName, ArchiveName,
                            ArchitectureName);
     }
 
     bool Global = Flags & SymbolRef::SF_Global;
     bool Weak = Flags & SymbolRef::SF_Weak;
     bool Absolute = Flags & SymbolRef::SF_Absolute;
     bool Common = Flags & SymbolRef::SF_Common;
     bool Hidden = Flags & SymbolRef::SF_Hidden;

 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()))
-        reportError(std::move(E), A->getFileName(), getFileNameForError(C, I));
+        reportError(std::move(E), getFileNameForError(C, I), A->getFileName());
       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
       reportError(errorCodeToError(object_error::invalid_file_type),