[DWARF] - Cleanup relocation handling.

[grimar]

Summary

Current version of Dwarf parser uses object::RelocVisitor for applying relocations.
And do that in a bit wierd way. It creates object::RelocVisitor object
for each relocation, though could do it once for file and reduce amount of checks, which
now happens because of visit() function that still check file type on each call and then
chooses which arch to use in a huge single method with use of lots helper function.

I was interested would it be any benefit if we change code to select proper architecture
and file type once during visitor creation. And implemented it for ELF x86_64 in this patch.

Numbers for me are 16,5909 ( +- 0,16% ) vs 16,3406 ( +- 0,19% ) for 25 runs of linking
llc binary with lld and -gdb-index. Difference is not much: 1,015, or up to 1% it seems for total link time.

Though this approach helps to split huge method and cleanup implementation.
Interesting what other thinks about that.

[dblaikie]

include/llvm/Object/RelocVisitor.h

53

Have you/could you try keeping the selection here, but using a switch over ObjToVisit.getType() rather than the if/else chain? (maybe that'll be easy enough to optimize & tidy things up rather than having to have the indirection through a function pointer.

[ruiu]

I think what we should do to tidy this class up is to remove a lot of visitELF_<ARCH> functions and inline them. Splitting into small function is usually good, but in this case it went too far.

[grimar]

• Addressed review comments.
• Rebased.

include/llvm/Object/RelocVisitor.h

53

Rui did a great cleenup here and it is looks much easier to avoid check of file type and target type on each relocation.
It just looks a bit wierd as obviously a useless job.

[ruiu]

include/llvm/Object/RelocVisitor.h

62

Why don't you use std::function?

[grimar]

include/llvm/Object/RelocVisitor.h

62

I am pretty sure I was asked by somebody to use poiner in one of reviews earlier, do not really remember why, so
I supposed it is preferred in LLVM before your comment, though it looks std::function is used widely. So I can switch to it.

[ruiu]

include/llvm/Object/RelocVisitor.h

62

Well, maybe a bare pointer is smaller and faster than std::function, I guess? I'd guess David Blaikie knows more than me about that.

[grimar]

Irrelevant.

include/llvm/Object/RelocVisitor.h

 #include <system_error>
 
 namespace llvm {
 namespace object {
 
 /// @brief Base class for object file relocation visitors.
 class RelocVisitor {
 public:
-  explicit RelocVisitor(const ObjectFile &Obj) : ObjToVisit(Obj) {}
+  explicit RelocVisitor(const ObjectFile &Obj) : ObjToVisit(Obj) {
+    if (isa<ELFObjectFileBase>(ObjToVisit))
+      Vis = getELFVisitor();
+    else if (isa<COFFObjectFile>(ObjToVisit))
+      Vis = &RelocVisitor::visitCOFF;
+    else if (isa<MachOObjectFile>(ObjToVisit))
+      Vis = &RelocVisitor::visitMachO;
+    else
+      llvm_unreachable("unable to create relocation visitor");
+  }
 
   // TODO: Should handle multiple applied relocations via either passing in the
   // previously computed value or just count paired relocations as a single
   // visit.
   uint64_t visit(uint32_t Rel, RelocationRef R, uint64_t Value = 0) {
-    if (isa<ELFObjectFileBase>(ObjToVisit))
-      return visitELF(Rel, R, Value);
-    if (isa<COFFObjectFile>(ObjToVisit))
-      return visitCOFF(Rel, R, Value);
-    if (isa<MachOObjectFile>(ObjToVisit))
-      return visitMachO(Rel, R, Value);
-
-    HasError = true;
-    return 0;
+    return (this->*Vis)(Rel, R, Value);
   }
 
   bool error() { return HasError; }
+  void clearError() { HasError = false; };
 
 private:
+  typedef uint64_t (RelocVisitor::*Visitor)(uint32_t RelocType, RelocationRef R,
+                                            uint64_t Value);
+  Visitor Vis;
+
   const ObjectFile &ObjToVisit;
   bool HasError = false;
 
-  uint64_t visitELF(uint32_t Rel, RelocationRef R, uint64_t Value) {
+  Visitor getELFVisitor() {
     if (ObjToVisit.getBytesInAddress() == 8) { // 64-bit object file
       switch (ObjToVisit.getArch()) {
       case Triple::x86_64:
-        return visitX86_64(Rel, R, Value);
+        return &RelocVisitor::visitX86_64;
       case Triple::aarch64:
       case Triple::aarch64_be:
-        return visitAarch64(Rel, R, Value);
+        return &RelocVisitor::visitAarch64;
       case Triple::bpfel:
       case Triple::bpfeb:
-        return visitBpf(Rel, R, Value);
+        return &RelocVisitor::visitBpf;
       case Triple::mips64el:
       case Triple::mips64:
-        return visitMips64(Rel, R, Value);
+        return &RelocVisitor::visitMips64;
       case Triple::ppc64le:
       case Triple::ppc64:
-        return visitPPC64(Rel, R, Value);
+        return &RelocVisitor::visitPPC64;
       case Triple::systemz:
-        return visitSystemz(Rel, R, Value);
+        return &RelocVisitor::visitSystemz;
       case Triple::sparcv9:
-        return visitSparc64(Rel, R, Value);
+        return &RelocVisitor::visitSparc64;
       case Triple::amdgcn:
-        return visitAmdgpu(Rel, R, Value);
+        return &RelocVisitor::visitAmdgpu;
       default:
         HasError = true;
         return 0;
       }
     }
 
     // 32-bit object file
     assert(ObjToVisit.getBytesInAddress() == 4 &&
            "Invalid word size in object file");
 
     switch (ObjToVisit.getArch()) {
     case Triple::x86:
-      return visitX86(Rel, R, Value);
+      return &RelocVisitor::visitX86;
     case Triple::ppc:
-      return visitPPC32(Rel, R, Value);
+      return &RelocVisitor::visitPPC32;
     case Triple::arm:
     case Triple::armeb:
-      return visitARM(Rel, R, Value);
+      return &RelocVisitor::visitARM;
     case Triple::lanai:
-      return visitLanai(Rel, R, Value);
+      return &RelocVisitor::visitLanai;
     case Triple::mipsel:
     case Triple::mips:
-      return visitMips32(Rel, R, Value);
+      return &RelocVisitor::visitMips32;
     case Triple::sparc:
-      return visitSparc32(Rel, R, Value);
+      return &RelocVisitor::visitSparc32;
     case Triple::hexagon:
-      return visitHexagon(Rel, R, Value);
+      return &RelocVisitor::visitHexagon;
     default:
       HasError = true;
       return 0;
     }
   }
 
   int64_t getELFAddend(RelocationRef R) {
     ErrorOr<int64_t> AddendOrErr = ELFRelocationRef(R).getAddend();

lib/DebugInfo/DWARF/DWARFContext.cpp

Error DWARFContextInMemory::maybeDecompress(const SectionRef &Sec,
 
   return Error::success();
 }
 
 DWARFContextInMemory::DWARFContextInMemory(const object::ObjectFile &Obj,
                                            const LoadedObjectInfo *L)
     : FileName(Obj.getFileName()), IsLittleEndian(Obj.isLittleEndian()),
       AddressSize(Obj.getBytesInAddress()) {
+  object::RelocVisitor V(Obj);
   for (const SectionRef &Section : Obj.sections()) {
     StringRef name;
     Section.getName(name);
     // Skip BSS and Virtual sections, they aren't interesting.
     bool IsBSS = Section.isBSS();
     if (IsBSS)
       continue;
     bool IsVirtual = Section.isVirtual();
for (const RelocationRef &Reloc : Section.relocations()) {
 
       Expected<uint64_t> SymAddrOrErr =
           getSymbolAddress(Obj, Reloc, L, AddrCache);
       if (!SymAddrOrErr) {
         errs() << toString(SymAddrOrErr.takeError()) << '\n';
         continue;
       }
 
-      object::RelocVisitor V(Obj);
       uint64_t Val = V.visit(Reloc.getType(), Reloc, *SymAddrOrErr);
       if (V.error()) {
         SmallString<32> Name;
         Reloc.getTypeName(Name);
         errs() << "error: failed to compute relocation: " << Name << "\n";
+        V.clearError();
         continue;
       }
       Map->insert({Reloc.getOffset(), {Val}});
     }
   }
 }
 
 DWARFContextInMemory::DWARFContextInMemory(