[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) {
+static void error(std::error_code EC) {
   if (!EC)
-    return false;
-
-  ReturnValue = EXIT_FAILURE;
+    return;
   outs() << "\nError reading file: " << EC.message() << ".\n";
   outs().flush();
-  return true;
+  exit(1);
 }
 
 } // namespace llvm
 
 static void reportError(StringRef Input, StringRef Message) {
   if (Input == "-")
     Input = "<stdin>";
-
   errs() << Input << ": " << Message << "\n";
   errs().flush();
-  ReturnValue = EXIT_FAILURE;
+  exit(1);
 }
 
 static void reportError(StringRef Input, std::error_code EC) {
   reportError(Input, EC.message());
 }
 
 static SmallVectorImpl<SectionRef> &getRelocSections(const ObjectFile *Obj,
                                                      const SectionRef &Sec) {
   static bool MappingDone = false;
   static std::map<SectionRef, SmallVector<SectionRef, 1>> SectionRelocMap;
   if (!MappingDone) {
     for (const SectionRef &Section : Obj->sections()) {
       section_iterator Sec2 = Section.getRelocatedSection();
       if (Sec2 != Obj->section_end())
         SectionRelocMap[*Sec2].push_back(Section);
     }
     MappingDone = true;
   }
   return SectionRelocMap[Sec];
 }
 
-static bool collectRelocatedSymbols(const ObjectFile *Obj,
+static void collectRelocatedSymbols(const ObjectFile *Obj,
                                     const SectionRef &Sec, uint64_t SecAddress,
                                     uint64_t SymAddress, uint64_t SymSize,
                                     StringRef *I, StringRef *E) {
   uint64_t SymOffset = SymAddress - SecAddress;
   uint64_t SymEnd = SymOffset + SymSize;
   for (const SectionRef &SR : getRelocSections(Obj, Sec)) {
     for (const object::RelocationRef &Reloc : SR.relocations()) {
       if (I == E)
         break;
       const object::symbol_iterator RelocSymI = Reloc.getSymbol();
       if (RelocSymI == Obj->symbol_end())
         continue;
       ErrorOr<StringRef> RelocSymName = RelocSymI->getName();
-      if (error(RelocSymName.getError()))
-        return true;
+      error(RelocSymName.getError());
       uint64_t Offset = Reloc.getOffset();
       if (Offset >= SymOffset && Offset < SymEnd) {
         *I = *RelocSymName;
         ++I;
       }
     }
   }
-  return false;
 }
 
-static bool collectRelocationOffsets(
+static void collectRelocationOffsets(
     const ObjectFile *Obj, const SectionRef &Sec, uint64_t SecAddress,
     uint64_t SymAddress, uint64_t SymSize, StringRef SymName,
     std::map<std::pair<StringRef, uint64_t>, StringRef> &Collection) {
   uint64_t SymOffset = SymAddress - SecAddress;
   uint64_t SymEnd = SymOffset + SymSize;
   for (const SectionRef &SR : getRelocSections(Obj, Sec)) {
     for (const object::RelocationRef &Reloc : SR.relocations()) {
       const object::symbol_iterator RelocSymI = Reloc.getSymbol();
       if (RelocSymI == Obj->symbol_end())
         continue;
       ErrorOr<StringRef> RelocSymName = RelocSymI->getName();
-      if (error(RelocSymName.getError()))
-        return true;
+      error(RelocSymName.getError());
       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) {
   struct CompleteObjectLocator {
     StringRef Symbols[2];
     ArrayRef<little32_t> Data;
   };
   struct ClassHierarchyDescriptor {
static void 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;
+    error(SymNameOrErr.getError());
     StringRef SymName = *SymNameOrErr;
     object::section_iterator SecI(Obj->section_begin());
-    if (error(Sym.getSection(SecI)))
-      return;
+    error(Sym.getSection(SecI));
     // 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;
+    error(Sec.getContents(SecContents));
     ErrorOr<uint64_t> SymAddressOrErr = Sym.getAddress();
-    if (error(SymAddressOrErr.getError()))
-      return;
+    error(SymAddressOrErr.getError());
     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
else if (SymName.startswith("??_8")) {
       VBTables[SymName] = VBTableData;
     }
     // 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;
+      collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I, E);
       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;
+      collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I, E);
       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;
+      collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I, E);
       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;
+      collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I, E);
       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")) {
           reinterpret_cast<const little32_t *>(SymContents.data());
       CatchableType 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;
+      collectRelocatedSymbols(Obj, Sec, SecAddress, SymAddress, SymSize, I, E);
       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'.
int main(int argc, const char *argv[]) {
 
   // 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;
+  return EXIT_SUCCESS;
 }