[llvm-cxxdump] Don't rely on global state

[davide]

Summary

It seems that llvm-cxxdump (and other tools) rely on a static global state for error. It doesn't seem great, and I can't see a reason why we need to do that (but I may miss it).
At least in the llvm-cxxdump case the conversion seems straightforward enough, so I tried to do it.

[davide]

Actually return EXIT_{FAILURE, SUCCESS} instead of false/true.

[• rafael]

Given what this tool is, can't we just call exit(1) from reportError?

[davide]

Take two.

[• rafael]

LGTM

llvm-cxxdump.cpp

 using namespace llvm::support;
 
 namespace opts {
 cl::list<std::string> InputFilenames(cl::Positional,
                                      cl::desc("<input object files>"),
                                      cl::ZeroOrMore);
 } // namespace opts
 
-static int ReturnValue = EXIT_SUCCESS;
-
 namespace llvm {
 
 static bool error(std::error_code EC) {
   if (!EC)
     return false;
 
-  ReturnValue = EXIT_FAILURE;
   outs() << "\nError reading file: " << EC.message() << ".\n";
   outs().flush();
   return true;
 }
 
 } // namespace llvm
 
 static void reportError(StringRef Input, StringRef Message) {
   if (Input == "-")
     Input = "<stdin>";
 
   errs() << Input << ": " << Message << "\n";
   errs().flush();
-  ReturnValue = EXIT_FAILURE;
 }
 
 static void reportError(StringRef Input, std::error_code EC) {
   reportError(Input, EC.message());
 }
 
 static SmallVectorImpl<SectionRef> &getRelocSections(const ObjectFile *Obj,
                                                      const SectionRef &Sec) {
for (const object::RelocationRef &Reloc : SR.relocations()) {
       uint64_t Offset = Reloc.getOffset();
       if (Offset >= SymOffset && Offset < SymEnd)
         Collection[std::make_pair(SymName, Offset - SymOffset)] = *RelocSymName;
     }
   }
   return false;
 }
 
-static void dumpCXXData(const ObjectFile *Obj) {
+static bool dumpCXXData(const ObjectFile *Obj) {
   struct CompleteObjectLocator {
     StringRef Symbols[2];
     ArrayRef<little32_t> Data;
   };
   struct ClassHierarchyDescriptor {
     StringRef Symbols[1];
     ArrayRef<little32_t> Data;
   };
static bool dumpCXXData(const ObjectFile *Obj) {
   std::vector<std::pair<SymbolRef, uint64_t>> SymAddr =
       object::computeSymbolSizes(*Obj);
 
   for (auto &P : SymAddr) {
     object::SymbolRef Sym = P.first;
     uint64_t SymSize = P.second;
     ErrorOr<StringRef> SymNameOrErr = Sym.getName();
     if (error(SymNameOrErr.getError()))
-      return;
+      return true;
     StringRef SymName = *SymNameOrErr;
     object::section_iterator SecI(Obj->section_begin());
     if (error(Sym.getSection(SecI)))
-      return;
+      return true;
     // Skip external symbols.
     if (SecI == Obj->section_end())
       continue;
     const SectionRef &Sec = *SecI;
     // Skip virtual or BSS sections.
     if (Sec.isBSS() || Sec.isVirtual())
       continue;
     StringRef SecContents;
     if (error(Sec.getContents(SecContents)))
-      return;
+      return true;
     ErrorOr<uint64_t> SymAddressOrErr = Sym.getAddress();
     if (error(SymAddressOrErr.getError()))
-      return;
+      return true;
     uint64_t SymAddress = *SymAddressOrErr;
     uint64_t SecAddress = Sec.getAddress();
     uint64_t SecSize = Sec.getSize();
     uint64_t SymOffset = SymAddress - SecAddress;
     StringRef SymContents = SecContents.substr(SymOffset, SymSize);
 
     // VFTables in the MS-ABI start with '??_7' and are contained within their
     // own COMDAT section.  We then determine the contents of the VFTable by
for (auto &P : SymAddr) {
     // Complete object locators in the MS-ABI start with '??_R4'
     else if (SymName.startswith("??_R4")) {
       CompleteObjectLocator COL;
       COL.Data = ArrayRef<little32_t>(
           reinterpret_cast<const little32_t *>(SymContents.data()), 3);
       StringRef *I = std::begin(COL.Symbols), *E = std::end(COL.Symbols);
       if (collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I,
                                   E))
-        return;
+        return true;
       COLs[SymName] = COL;
     }
     // Class hierarchy descriptors in the MS-ABI start with '??_R3'
     else if (SymName.startswith("??_R3")) {
       ClassHierarchyDescriptor CHD;
       CHD.Data = ArrayRef<little32_t>(
           reinterpret_cast<const little32_t *>(SymContents.data()), 3);
       StringRef *I = std::begin(CHD.Symbols), *E = std::end(CHD.Symbols);
       if (collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I,
                                   E))
-        return;
+        return true;
       CHDs[SymName] = CHD;
     }
     // Class hierarchy descriptors in the MS-ABI start with '??_R2'
     else if (SymName.startswith("??_R2")) {
       // Each relocation names a base class descriptor.  We note the offset into
       // the section and the symbol used for the relocation.
       collectRelocationOffsets(Obj, Sec, SecAddress, SymAddress, SymSize,
                                SymName, BCAEntries);
     }
     // Base class descriptors in the MS-ABI start with '??_R1'
     else if (SymName.startswith("??_R1")) {
       BaseClassDescriptor BCD;
       BCD.Data = ArrayRef<little32_t>(
           reinterpret_cast<const little32_t *>(SymContents.data()) + 1, 5);
       StringRef *I = std::begin(BCD.Symbols), *E = std::end(BCD.Symbols);
       if (collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I,
                                   E))
-        return;
+        return true;
       BCDs[SymName] = BCD;
     }
     // Type descriptors in the MS-ABI start with '??_R0'
     else if (SymName.startswith("??_R0")) {
       const char *DataPtr = SymContents.drop_front(BytesInAddress).data();
       TypeDescriptor TD;
       if (BytesInAddress == 8)
         TD.AlwaysZero = *reinterpret_cast<const little64_t *>(DataPtr);
       else
         TD.AlwaysZero = *reinterpret_cast<const little32_t *>(DataPtr);
       TD.MangledName = SymContents.drop_front(BytesInAddress * 2);
       StringRef *I = std::begin(TD.Symbols), *E = std::end(TD.Symbols);
       if (collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I,
                                   E))
-        return;
+        return true;
       TDs[SymName] = TD;
     }
     // Throw descriptors in the MS-ABI start with '_TI'
     else if (SymName.startswith("_TI") || SymName.startswith("__TI")) {
       ThrowInfo TI;
       TI.Flags = *reinterpret_cast<const little32_t *>(SymContents.data());
       collectRelocationOffsets(Obj, Sec, SecAddress, SymAddress, SymSize,
                                SymName, TIEntries);
else if (SymName.startswith("_CT") || SymName.startswith("__CT")) {
       CT.Flags = DataPtr[0];
       CT.NonVirtualBaseAdjustmentOffset = DataPtr[2];
       CT.VirtualBasePointerOffset = DataPtr[3];
       CT.VirtualBaseAdjustmentOffset = DataPtr[4];
       CT.Size = DataPtr[5];
       StringRef *I = std::begin(CT.Symbols), *E = std::end(CT.Symbols);
       if (collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I,
                                   E))
-        return;
+        return true;
       CTs[SymName] = CT;
     }
     // Construction vtables in the Itanium ABI start with '_ZTT' or '__ZTT'.
     else if (SymName.startswith("_ZTT") || SymName.startswith("__ZTT")) {
       collectRelocationOffsets(Obj, Sec, SecAddress, SymAddress, SymSize,
                                SymName, VTTEntries);
     }
     // Typeinfo names in the Itanium ABI start with '_ZTS' or '__ZTS'.
if (!DataDone && (SymDone || VTableDataI->first < VTableSymI->first)) {
       int64_t VTableEntry = VTableDataI->second;
       outs() << VTableName << '[' << Offset << "]: ";
       outs() << VTableEntry;
       outs() << '\n';
       ++VTableDataI;
       continue;
     }
   }
+  return false;
 }
 
-static void dumpArchive(const Archive *Arc) {
+static bool dumpArchive(const Archive *Arc) {
+  bool Fail = false;
   for (const Archive::Child &ArcC : Arc->children()) {
     ErrorOr<std::unique_ptr<Binary>> ChildOrErr = ArcC.getAsBinary();
     if (std::error_code EC = ChildOrErr.getError()) {
       // Ignore non-object files.
-      if (EC != object_error::invalid_file_type)
+      if (EC != object_error::invalid_file_type) {
         reportError(Arc->getFileName(), EC.message());
+        Fail = true;
+      }
       continue;
     }
 
     if (ObjectFile *Obj = dyn_cast<ObjectFile>(&*ChildOrErr.get()))
       dumpCXXData(Obj);
-    else
+    else {
       reportError(Arc->getFileName(), cxxdump_error::unrecognized_file_format);
+      Fail = true;
+    }
   }
+  return Fail;
 }
 
-static void dumpInput(StringRef File) {
+static bool dumpInput(StringRef File) {
   // If file isn't stdin, check that it exists.
   if (File != "-" && !sys::fs::exists(File)) {
     reportError(File, cxxdump_error::file_not_found);
-    return;
+    return true;
   }
 
   // Attempt to open the binary.
   ErrorOr<OwningBinary<Binary>> BinaryOrErr = createBinary(File);
   if (std::error_code EC = BinaryOrErr.getError()) {
     reportError(File, EC);
-    return;
+    return true;
   }
   Binary &Binary = *BinaryOrErr.get().getBinary();
-
+  bool Fail;
   if (Archive *Arc = dyn_cast<Archive>(&Binary))
-    dumpArchive(Arc);
+    Fail = dumpArchive(Arc);
   else if (ObjectFile *Obj = dyn_cast<ObjectFile>(&Binary))
-    dumpCXXData(Obj);
-  else
+    Fail = dumpCXXData(Obj);
+  else {
     reportError(File, cxxdump_error::unrecognized_file_format);
+    return true;
+  }
+  return Fail;
 }
 
 int main(int argc, const char *argv[]) {
   sys::PrintStackTraceOnErrorSignal();
   PrettyStackTraceProgram X(argc, argv);
   llvm_shutdown_obj Y;
+  bool Fail;
 
   // Initialize targets.
   llvm::InitializeAllTargetInfos();
 
   // Register the target printer for --version.
   cl::AddExtraVersionPrinter(TargetRegistry::printRegisteredTargetsForVersion);
 
   cl::ParseCommandLineOptions(argc, argv, "LLVM C++ ABI Data Dumper\n");
 
   // Default to stdin if no filename is specified.
   if (opts::InputFilenames.size() == 0)
     opts::InputFilenames.push_back("-");
 
-  std::for_each(opts::InputFilenames.begin(), opts::InputFilenames.end(),
-                dumpInput);
-
-  return ReturnValue;
+  for (StringRef F: opts::InputFilenames)
+    if (dumpInput(F))
+      Fail = true;
+  return Fail;
 }