[DWARF] Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries

[MaskRay]

Summary

getRelocatedValue may compute incorrect value for SHT_RELA-typed relocation entries.

// DWARFDataExtractor.cpp
uint64_t DWARFDataExtractor::getRelocatedValue(uint32_t Size, uint32_t *Off,
...

// This formula is correct for REL, but may be incorrect for RELA if the value
// stored in the location (getUnsigned(Off, Size)) is not zero.
return getUnsigned(Off, Size) + Rel->Value;

In this patch, we

• refactor these visit* functions to include a new parameter uint64_t A. Since these visit* functions are no longer used as visitors, rename them to resolve*. + REL: A is used as the addend. A is the value stored in the location where the relocation applies: getUnsigned(Off, Size) + RELA: The addend encoded in RelocationRef is used, e.g. getELFAddend(R)
• and add another set of supports* functions to check if a given relocation type is handled. DWARFObjInMemory uses them to fail early.

[dblaikie]

This is a bit hard to follow in Phab due to the file rename (it looks like all the code is "new" when it isn't) - would it be reasonable to do this without the file rename first, then rename the file in a follow-up commit?

[MaskRay]

RelocVisitor.h -> RelocationResolver.cpp there are so many differences that git doesn't consider it a rename.

git diff 'HEAD^':include/llvm/Object/RelocVisitor.h lib/Object/RelocationResolver.cpp

The member functions visit* have been made static functions. The changed indentation, changed signature probably make git think they are different chunks.

a.diff17 KBDownload

Anyway, we can see from below the structure of the architecture dispatch is kept.

patch
+      switch (Obj.getArch()) {
       case Triple::x86_64:
-        return visitX86_64(Rel, R, Value);
+        return resolveX86_64;
       case Triple::aarch64:
       case Triple::aarch64_be:
-        return visitAarch64(Rel, R, Value);
+        return resolveAarch64;
       case Triple::bpfel:
       case Triple::bpfeb:
-        return visitBpf(Rel, R, Value);
+        return resolveBpf;
       case Triple::mips64el:
       case Triple::mips64:
-        return visitMips64(Rel, R, Value);
+        return resolveMips64;
       case Triple::ppc64le:
       case Triple::ppc64:
-        return visitPPC64(Rel, R, Value);
+        return resolvePPC64;
       case Triple::systemz:
-        return visitSystemz(Rel, R, Value);
+        return resolveSystemz;
       case Triple::sparcv9:
-        return visitSparc64(Rel, R, Value);
+        return resolveSparc64;
       case Triple::amdgcn:
-        return visitAmdgpu(Rel, R, Value);
+        return resolveAmdgpu;
       default:
-        HasError = true;
-        return 0;
+        return nullptr;
       }
     }

[dblaikie]

In D57939#1391531, @MaskRay wrote:

RelocVisitor.h -> RelocationResolver.cpp there are so many differences that git doesn't consider it a rename.

Right - what I'm asking is if you could undo/not apply the file name change in this revision, so the patch/review is easier to read, possibly? (you could undo the file name change & upload a new revision for this review - then the delta might be smaller/easier to read)

git diff 'HEAD^':include/llvm/Object/RelocVisitor.h lib/Object/RelocationResolver.cpp

The member functions visit* have been made static functions. The changed indentation, changed signature probably make git think they are different chunks.

a.diff17 KBDownload

Anyway, we can see from below the structure of the architecture dispatch is kept.

patch
+      switch (Obj.getArch()) {
       case Triple::x86_64:
-        return visitX86_64(Rel, R, Value);
+        return resolveX86_64;
       case Triple::aarch64:
       case Triple::aarch64_be:
-        return visitAarch64(Rel, R, Value);
+        return resolveAarch64;
       case Triple::bpfel:
       case Triple::bpfeb:
-        return visitBpf(Rel, R, Value);
+        return resolveBpf;
       case Triple::mips64el:
       case Triple::mips64:
-        return visitMips64(Rel, R, Value);
+        return resolveMips64;
       case Triple::ppc64le:
       case Triple::ppc64:
-        return visitPPC64(Rel, R, Value);
+        return resolvePPC64;
       case Triple::systemz:
-        return visitSystemz(Rel, R, Value);
+        return resolveSystemz;
       case Triple::sparcv9:
-        return visitSparc64(Rel, R, Value);
+        return resolveSparc64;
       case Triple::amdgcn:
-        return visitAmdgpu(Rel, R, Value);
+        return resolveAmdgpu;
       default:
-        HasError = true;
-        return 0;
+        return nullptr;
       }
     }

[MaskRay]

In D57939#1391941, @dblaikie wrote:

In D57939#1391531, @MaskRay wrote:

RelocVisitor.h -> RelocationResolver.cpp there are so many differences that git doesn't consider it a rename.

Right - what I'm asking is if you could undo/not apply the file name change in this revision, so the patch/review is easier to read, possibly? (you could undo the file name change & upload a new revision for this review - then the delta might be smaller/easier to read)

I just created https://reviews.llvm.org/D58020 for exposition purposes. This patch and D58020 have no functional difference but D58020 puts all changes together in RelocVisitor.h

[dblaikie]

include/llvm/DebugInfo/DWARF/DWARFRelocMap.h

24

Could this have a more verbose/self-descriptive name?

lib/DebugInfo/DWARF/DWARFDataExtractor.cpp

27

Could RelocAddrEntry maybe have a "resolve" function that handles most of this? (eg: passes in E->Reloc and E->S, handles the "getValueOr" part too, probably - and/or maybe the "getValueOr" part could be handled by having a default/trivial Resolver that returns A? Less branching in the code required that way)

[MaskRay]

make Resolver* more efficient

[MaskRay]

include/llvm/DebugInfo/DWARF/DWARFRelocMap.h

24

Changed it to SymbolValue.

In ELF ABI, this is typically: "S Represents the value of the symbol whose index resides in the relocation entry"

lib/DebugInfo/DWARF/DWARFDataExtractor.cpp

27

Changed it to return E->Resolver(E->Reloc, E->SymbolValue, A);

The check (whether Resolver can handle the relocation type) in DWARFContext.cpp is gross but efficient..

[dblaikie]

lib/DebugInfo/DWARF/DWARFContext.cpp

1512–1518

What's the failure mode if this is not diagnosed here?

If there's a valid fallback/reliable failure later - could we remove this code entirely? (rather than having a best effort then fallback - seems like having two partial solutions in different places is liable to have things slip through)

[MaskRay]

lib/DebugInfo/DWARF/DWARFContext.cpp

1512–1518

What's the failure mode if this is not diagnosed here?

The unresolved relocation type will slip through and there will be no diagnostics later when the relocations are actually resolved. (Note that the return types of visit* (or resolve* after the patch) are uint64_t, instead of Error Optional or others).

And the usage (call sites of getRelocatedValue) does not allow it to do fancy error handling.

uint64_t DWARFDataExtractor::getRelocatedValue(uint32_t Size, uint32_t *Off,
                                               uint64_t *SecNdx) const {
  if (SecNdx)
    *SecNdx = -1ULL;
  if (!Section)
    return getUnsigned(Off, Size);
  Optional<RelocAddrEntry> Rel = Obj->find(*Section, *Off);
  if (!Rel)
    return getUnsigned(Off, Size);
  if (SecNdx)
    *SecNdx = Rel->SectionIndex;
  return getUnsigned(Off, Size) + Rel->Value;
}

So I know that this is ugly and can be improved.. But to fix the bug I cannot think of a better way without being more intrusive (I do not want to touch many other places for this patch).

[dblaikie]

lib/DebugInfo/DWARF/DWARFContext.cpp

1512–1518

OK, it sounds like the comment might be a bit off, then:

" It isn't a big deal if this check fails. We just fail to catch this error early."

That last bit sounds like "well, we don't catch it /early/, I guess we catch it later" - but it sounds like we don't catch it later? So I'm not sure if it's not a big deal.

I'm not sure the partial solution is ideal either, as it seems to make it harder to see that it's incomplete/still problematic. Perhaps it'd be better to remove those Resolver -1ULL calls to demonstrate the general failure & implement a general fix - maybe not in this patch, maybe in a precursor or follow-up. Not sure about breaking this temporarily rather than laying out the patches to fix this without a period of regression.

@echristo - got any thoughts on this?

[MaskRay]

Update the comments as the old comments might be a bit off.

[echristo]

lib/DebugInfo/DWARF/DWARFContext.cpp

1512–1518

Taking a look through this ... we've got terrible error handling here. I'm also not a huge fan of storing the resolver inside every part of the map. That said as an incremental step it's not too bad as long as we document it a bit better explaining that the -{1,2,3}ULL mean in the if statement at what the purpose is.

Then we can probably refactor some of our relocation handling to be able to better check valid relocation first and then go from there.

WDYT?

[MaskRay]

Better comment

[dblaikie]

lib/DebugInfo/DWARF/DWARFContext.cpp

1512–1518

Taking a look through this ... we've got terrible error handling here. I'm also not a huge fan of storing the resolver inside every part of the map. That said as an incremental step it's not too bad as long as we document it a bit better explaining that the -{1,2,3}ULL mean in the if statement at what the purpose is.

I think the bit I'm uncertain about is that this seems like a step backwards in terms of error coverage - not quite incremental progress. But if the existing error coverage isn't reliable in some way (it's not clear to me that it is) - yeah, I don't have strong feelings about preserving it.

Then we can probably refactor some of our relocation handling to be able to better check valid relocation first and then go from there.

If we can refactor the error checking first so it's preserved through this change, that sounds fine.

Validating relocations up-front might be problematic from a performance perspective ( comes back to some of the design discussions around error handling in lld, etc - up-front error checking that might check too many things (when some sections are discarded, etc)).

[MaskRay]

Add supports*

[MaskRay]

lib/DebugInfo/DWARF/DWARFContext.cpp

1512–1518

Deleted -2/-3 hack and added supports* functions.

[echristo]

The duplication is a little sad, but I don't have a lot of ideas on how to remove that in the near term so LGTM. Dave, any other concerns on your end?

[MaskRay]

Update description

[MaskRay]

Update title

include/llvm/DebugInfo/DWARF/DWARFRelocMap.h

 //===- DWARFRelocMap.h ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEBUGINFO_DWARF_DWARFRELOCMAP_H
 #define LLVM_DEBUGINFO_DWARF_DWARFRELOCMAP_H
 
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/Object/RelocationResolver.h"
 #include <cstdint>
 
 namespace llvm {
 
 /// RelocAddrEntry contains relocated value and section index.
 /// Section index is -1LL if relocation points to absolute symbol.
 struct RelocAddrEntry {
   uint64_t SectionIndex;
-  uint64_t Value;
+  object::RelocationRef Reloc;
+  object::RelocationResolver Resolver;
+  uint64_t S;
 };
 
 /// In place of applying the relocations to the data we've read from disk we use
 /// a separate mapping table to the side and checking that at locations in the
 /// dwarf where we expect relocated values. This adds a bit of complexity to the
 /// dwarf parsing/extraction at the benefit of not allocating memory for the
 /// entire size of the debug info sections.
 using RelocAddrMap = DenseMap<uint64_t, RelocAddrEntry>;
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFRELOCMAP_H

include/llvm/Object/RelocVisitor.h

-//===- RelocVisitor.h - Visitor for object file relocations -----*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file provides a wrapper around all the different types of relocations
-// in different file formats, such that a client can handle them in a unified
-// manner by only implementing a minimal number of functions.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_OBJECT_RELOCVISITOR_H
-#define LLVM_OBJECT_RELOCVISITOR_H
-
-#include "llvm/ADT/Triple.h"
-#include "llvm/BinaryFormat/ELF.h"
-#include "llvm/BinaryFormat/MachO.h"
-#include "llvm/Object/COFF.h"
-#include "llvm/Object/ELFObjectFile.h"
-#include "llvm/Object/MachO.h"
-#include "llvm/Object/ObjectFile.h"
-#include "llvm/Object/Wasm.h"
-#include "llvm/Support/Casting.h"
-#include "llvm/Support/ErrorHandling.h"
-#include <cstdint>
-#include <system_error>
-
-namespace llvm {
-namespace object {
-
-/// Base class for object file relocation visitors.
-class RelocVisitor {
-public:
-  explicit RelocVisitor(const ObjectFile &Obj) : ObjToVisit(Obj) {}
-
-  // 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);
-    if (isa<WasmObjectFile>(ObjToVisit))
-      return visitWasm(Rel, R, Value);
-
-    HasError = true;
-    return 0;
-  }
-
-  bool error() { return HasError; }
-
-private:
-  const ObjectFile &ObjToVisit;
-  bool HasError = false;
-
-  uint64_t visitELF(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (ObjToVisit.getBytesInAddress() == 8) { // 64-bit object file
-      switch (ObjToVisit.getArch()) {
-      case Triple::x86_64:
-        return visitX86_64(Rel, R, Value);
-      case Triple::aarch64:
-      case Triple::aarch64_be:
-        return visitAarch64(Rel, R, Value);
-      case Triple::bpfel:
-      case Triple::bpfeb:
-        return visitBpf(Rel, R, Value);
-      case Triple::mips64el:
-      case Triple::mips64:
-        return visitMips64(Rel, R, Value);
-      case Triple::ppc64le:
-      case Triple::ppc64:
-        return visitPPC64(Rel, R, Value);
-      case Triple::systemz:
-        return visitSystemz(Rel, R, Value);
-      case Triple::sparcv9:
-        return visitSparc64(Rel, R, Value);
-      case Triple::amdgcn:
-        return visitAmdgpu(Rel, R, Value);
-      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);
-    case Triple::ppc:
-      return visitPPC32(Rel, R, Value);
-    case Triple::arm:
-    case Triple::armeb:
-      return visitARM(Rel, R, Value);
-    case Triple::avr:
-      return visitAVR(Rel, R, Value);
-    case Triple::lanai:
-      return visitLanai(Rel, R, Value);
-    case Triple::mipsel:
-    case Triple::mips:
-      return visitMips32(Rel, R, Value);
-    case Triple::sparc:
-      return visitSparc32(Rel, R, Value);
-    case Triple::hexagon:
-      return visitHexagon(Rel, R, Value);
-    default:
-      HasError = true;
-      return 0;
-    }
-  }
-
-  int64_t getELFAddend(RelocationRef R) {
-    Expected<int64_t> AddendOrErr = ELFRelocationRef(R).getAddend();
-    handleAllErrors(AddendOrErr.takeError(), [](const ErrorInfoBase &EI) {
-      report_fatal_error(EI.message());
-    });
-    return *AddendOrErr;
-  }
-
-  uint64_t visitX86_64(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_X86_64_NONE:
-      return 0;
-    case ELF::R_X86_64_64:
-    case ELF::R_X86_64_DTPOFF32:
-    case ELF::R_X86_64_DTPOFF64:
-      return Value + getELFAddend(R);
-    case ELF::R_X86_64_PC32:
-      return Value + getELFAddend(R) - R.getOffset();
-    case ELF::R_X86_64_32:
-    case ELF::R_X86_64_32S:
-      return (Value + getELFAddend(R)) & 0xFFFFFFFF;
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitAarch64(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_AARCH64_ABS32: {
-      int64_t Res = Value + getELFAddend(R);
-      if (Res < INT32_MIN || Res > UINT32_MAX)
-        HasError = true;
-      return static_cast<uint32_t>(Res);
-    }
-    case ELF::R_AARCH64_ABS64:
-      return Value + getELFAddend(R);
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitBpf(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_BPF_64_32:
-      return Value & 0xFFFFFFFF;
-    case ELF::R_BPF_64_64:
-      return Value;
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitMips64(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_MIPS_32:
-      return (Value + getELFAddend(R)) & 0xFFFFFFFF;
-    case ELF::R_MIPS_64:
-      return Value + getELFAddend(R);
-    case ELF::R_MIPS_TLS_DTPREL64:
-      return Value + getELFAddend(R) - 0x8000;
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitPPC64(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_PPC64_ADDR32:
-      return (Value + getELFAddend(R)) & 0xFFFFFFFF;
-    case ELF::R_PPC64_ADDR64:
-      return Value + getELFAddend(R);
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitSystemz(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_390_32: {
-      int64_t Res = Value + getELFAddend(R);
-      if (Res < INT32_MIN || Res > UINT32_MAX)
-        HasError = true;
-      return static_cast<uint32_t>(Res);
-    }
-    case ELF::R_390_64:
-      return Value + getELFAddend(R);
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitSparc64(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_SPARC_32:
-    case ELF::R_SPARC_64:
-    case ELF::R_SPARC_UA32:
-    case ELF::R_SPARC_UA64:
-      return Value + getELFAddend(R);
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitAmdgpu(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_AMDGPU_ABS32:
-    case ELF::R_AMDGPU_ABS64:
-      return Value + getELFAddend(R);
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitX86(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (Rel) {
-    case ELF::R_386_NONE:
-      return 0;
-    case ELF::R_386_32:
-      return Value;
-    case ELF::R_386_PC32:
-      return Value - R.getOffset();
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitPPC32(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (Rel == ELF::R_PPC_ADDR32)
-      return (Value + getELFAddend(R)) & 0xFFFFFFFF;
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitARM(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (Rel == ELF::R_ARM_ABS32) {
-      if ((int64_t)Value < INT32_MIN || (int64_t)Value > UINT32_MAX)
-        HasError = true;
-      return static_cast<uint32_t>(Value);
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitAVR(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (Rel == ELF::R_AVR_16) {
-      return (Value + getELFAddend(R)) & 0xFFFF;
-    } else if (Rel == ELF::R_AVR_32) {
-      return (Value + getELFAddend(R)) & 0xFFFFFFFF;
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitLanai(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (Rel == ELF::R_LANAI_32)
-      return (Value + getELFAddend(R)) & 0xFFFFFFFF;
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitMips32(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    // FIXME: Take in account implicit addends to get correct results.
-    if (Rel == ELF::R_MIPS_32)
-      return Value & 0xFFFFFFFF;
-    if (Rel == ELF::R_MIPS_TLS_DTPREL32)
-      return Value & 0xFFFFFFFF;
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitSparc32(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (Rel == ELF::R_SPARC_32 || Rel == ELF::R_SPARC_UA32)
-      return Value + getELFAddend(R);
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitHexagon(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (Rel == ELF::R_HEX_32)
-      return Value + getELFAddend(R);
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitCOFF(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    switch (ObjToVisit.getArch()) {
-    case Triple::x86:
-      switch (Rel) {
-      case COFF::IMAGE_REL_I386_SECREL:
-      case COFF::IMAGE_REL_I386_DIR32:
-        return static_cast<uint32_t>(Value);
-      }
-      break;
-    case Triple::x86_64:
-      switch (Rel) {
-      case COFF::IMAGE_REL_AMD64_SECREL:
-        return static_cast<uint32_t>(Value);
-      case COFF::IMAGE_REL_AMD64_ADDR64:
-        return Value;
-      }
-      break;
-    default:
-      break;
-    }
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitMachO(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (ObjToVisit.getArch() == Triple::x86_64 &&
-        Rel == MachO::X86_64_RELOC_UNSIGNED)
-      return Value;
-    HasError = true;
-    return 0;
-  }
-
-  uint64_t visitWasm(uint32_t Rel, RelocationRef R, uint64_t Value) {
-    if (ObjToVisit.getArch() == Triple::wasm32) {
-      switch (Rel) {
-      case wasm::R_WASM_FUNCTION_INDEX_LEB:
-      case wasm::R_WASM_TABLE_INDEX_SLEB:
-      case wasm::R_WASM_TABLE_INDEX_I32:
-      case wasm::R_WASM_MEMORY_ADDR_LEB:
-      case wasm::R_WASM_MEMORY_ADDR_SLEB:
-      case wasm::R_WASM_MEMORY_ADDR_I32:
-      case wasm::R_WASM_TYPE_INDEX_LEB:
-      case wasm::R_WASM_GLOBAL_INDEX_LEB:
-      case wasm::R_WASM_FUNCTION_OFFSET_I32:
-      case wasm::R_WASM_SECTION_OFFSET_I32:
-      case wasm::R_WASM_EVENT_INDEX_LEB:
-        // For wasm section, its offset at 0 -- ignoring Value
-        return 0;
-      }
-    }
-    HasError = true;
-    return 0;
-  }
-};
-
-} // end namespace object
-} // end namespace llvm
-
-#endif // LLVM_OBJECT_RELOCVISITOR_H

include/llvm/Object/RelocationResolver.h

+//===- RelocVisitor.h - Visitor for object file relocations -----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides a wrapper around all the different types of relocations
+// in different file formats, such that a client can handle them in a unified
+// manner by only implementing a minimal number of functions.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_OBJECT_RELOCVISITOR_H
+#define LLVM_OBJECT_RELOCVISITOR_H
+
+#include "llvm/ADT/Triple.h"
+#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/BinaryFormat/MachO.h"
+#include "llvm/Object/COFF.h"
+#include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Object/MachO.h"
+#include "llvm/Object/ObjectFile.h"
+#include "llvm/Object/Wasm.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/ErrorHandling.h"
+#include <cstdint>
+#include <system_error>
+
+namespace llvm {
+namespace object {
+
+using RelocationResolver = Optional<uint64_t> (*)(RelocationRef R, uint64_t S,
+                                                  int64_t A);
+
+RelocationResolver getRelocationResolver(const ObjectFile &Obj);
+
+} // end namespace object
+} // end namespace llvm
+
+#endif // LLVM_OBJECT_RELOCVISITOR_H

lib/DebugInfo/DWARF/DWARFContext.cpp

 #include "llvm/DebugInfo/DWARF/DWARFGdbIndex.h"
 #include "llvm/DebugInfo/DWARF/DWARFSection.h"
 #include "llvm/DebugInfo/DWARF/DWARFUnitIndex.h"
 #include "llvm/DebugInfo/DWARF/DWARFVerifier.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/Object/Decompressor.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/ObjectFile.h"
-#include "llvm/Object/RelocVisitor.h"
+#include "llvm/Object/RelocationResolver.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/WithColor.h"
for (const SectionRef &Section : Obj.sections()) {
           continue;
       }
 
       if (Section.relocation_begin() == Section.relocation_end())
         continue;
 
       // Symbol to [address, section index] cache mapping.
       std::map<SymbolRef, SymInfo> AddrCache;
+      RelocationResolver Resolver = getRelocationResolver(Obj);
       for (const RelocationRef &Reloc : Section.relocations()) {
         // FIXME: it's not clear how to correctly handle scattered
         // relocations.
         if (isRelocScattered(Obj, Reloc))
           continue;
 
         Expected<SymInfo> SymInfoOrErr =
             getSymbolInfo(Obj, Reloc, L, AddrCache);
         if (!SymInfoOrErr) {
           if (HandleError(SymInfoOrErr.takeError()) == ErrorPolicy::Halt)
             return;
           continue;
         }
 
-        object::RelocVisitor V(Obj);
-        uint64_t Val = V.visit(Reloc.getType(), Reloc, SymInfoOrErr->Address);
-        if (V.error()) {
+        if (Optional<uint64_t> Val =
+                Resolver ? Resolver(Reloc, SymInfoOrErr->Address, 0) : None) {
+          Map->try_emplace(Reloc.getOffset(),
+                           RelocAddrEntry{SymInfoOrErr->SectionIndex, Reloc,
+                                          Resolver, SymInfoOrErr->Address});
+        } else {
           SmallString<32> Type;
           Reloc.getTypeName(Type);
           ErrorPolicy EP = HandleError(
               createError("failed to compute relocation: " + Type + ", ",
                           errorCodeToError(object_error::parse_failed)));
           if (EP == ErrorPolicy::Halt)
             return;
-          continue;
         }
-        RelocAddrEntry Rel = {SymInfoOrErr->SectionIndex, Val};
-        Map->insert({Reloc.getOffset(), Rel});
       }
     }
 
     for (SectionName &S : SectionNames)
       if (SectionAmountMap[S.Name] > 1)
         S.IsNameUnique = false;
   }
 

lib/DebugInfo/DWARF/DWARFDataExtractor.cpp

 using namespace llvm;
 
 uint64_t DWARFDataExtractor::getRelocatedValue(uint32_t Size, uint32_t *Off,
                                                uint64_t *SecNdx) const {
   if (SecNdx)
     *SecNdx = -1ULL;
   if (!Section)
     return getUnsigned(Off, Size);
-  Optional<RelocAddrEntry> Rel = Obj->find(*Section, *Off);
-  if (!Rel)
-    return getUnsigned(Off, Size);
+  Optional<RelocAddrEntry> E = Obj->find(*Section, *Off);
+  uint64_t A = getUnsigned(Off, Size);
+  if (!E)
+    return A;
   if (SecNdx)
-    *SecNdx = Rel->SectionIndex;
-  return getUnsigned(Off, Size) + Rel->Value;
+    *SecNdx = E->SectionIndex;
+  return E->Resolver(E->Reloc, E->S, A).getValueOr(A);
 }
 
 Optional<uint64_t>
 DWARFDataExtractor::getEncodedPointer(uint32_t *Offset, uint8_t Encoding,
                                       uint64_t PCRelOffset) const {
   if (Encoding == dwarf::DW_EH_PE_omit)
     return None;
 

lib/Object/CMakeLists.txt

add_llvm_library(LLVMObject
   IRObjectFile.cpp
   IRSymtab.cpp
   MachOObjectFile.cpp
   MachOUniversal.cpp
   ModuleSymbolTable.cpp
   Object.cpp
   ObjectFile.cpp
   RecordStreamer.cpp
+  RelocationResolver.cpp
   SymbolicFile.cpp
   SymbolSize.cpp
   WasmObjectFile.cpp
   WindowsResource.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Object
 
   DEPENDS
   intrinsics_gen
   llvm_vcsrevision_h
   )

lib/Object/RelocationResolver.cpp

+//===- RelocationResolver.cpp ------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines utilities to resolve relocations in object files.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Object/RelocationResolver.h"
+
+namespace llvm {
+namespace object {
+
+static int64_t getELFAddend(RelocationRef R) {
+  Expected<int64_t> AddendOrErr = ELFRelocationRef(R).getAddend();
+  handleAllErrors(AddendOrErr.takeError(), [](const ErrorInfoBase &EI) {
+    report_fatal_error(EI.message());
+  });
+  return *AddendOrErr;
+}
+
+static Optional<uint64_t> resolveX86_64(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_X86_64_NONE:
+    return A;
+  case ELF::R_X86_64_64:
+  case ELF::R_X86_64_DTPOFF32:
+  case ELF::R_X86_64_DTPOFF64:
+    return S + getELFAddend(R);
+  case ELF::R_X86_64_PC32:
+    return S + getELFAddend(R) - R.getOffset();
+  case ELF::R_X86_64_32:
+  case ELF::R_X86_64_32S:
+    return (S + getELFAddend(R)) & 0xFFFFFFFF;
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveAarch64(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_AARCH64_ABS32: {
+    int64_t Res = S + getELFAddend(R);
+    if (Res < INT32_MIN || Res > UINT32_MAX)
+      return None;
+    return static_cast<uint32_t>(Res);
+  }
+  case ELF::R_AARCH64_ABS64:
+    return S + getELFAddend(R);
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveBpf(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_BPF_64_32:
+    return S & 0xFFFFFFFF;
+  case ELF::R_BPF_64_64:
+    return S;
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveMips64(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_MIPS_32:
+    return (S + getELFAddend(R)) & 0xFFFFFFFF;
+  case ELF::R_MIPS_64:
+    return S + getELFAddend(R);
+  case ELF::R_MIPS_TLS_DTPREL64:
+    return S + getELFAddend(R) - 0x8000;
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolvePPC64(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_PPC64_ADDR32:
+    return (S + getELFAddend(R)) & 0xFFFFFFFF;
+  case ELF::R_PPC64_ADDR64:
+    return S + getELFAddend(R);
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveSystemz(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_390_32: {
+    int64_t Res = S + getELFAddend(R);
+    if (Res < INT32_MIN || Res > UINT32_MAX)
+      return None;
+    return static_cast<uint32_t>(Res);
+  }
+  case ELF::R_390_64:
+    return S + getELFAddend(R);
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveSparc64(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_SPARC_32:
+  case ELF::R_SPARC_64:
+  case ELF::R_SPARC_UA32:
+  case ELF::R_SPARC_UA64:
+    return S + getELFAddend(R);
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveAmdgpu(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_AMDGPU_ABS32:
+  case ELF::R_AMDGPU_ABS64:
+    return S + getELFAddend(R);
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveX86(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_386_NONE:
+    return A;
+  case ELF::R_386_32:
+    return S + A;
+  case ELF::R_386_PC32:
+    return S - R.getOffset() + A;
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolvePPC32(RelocationRef R, uint64_t S, int64_t A) {
+  if (R.getType() == ELF::R_PPC_ADDR32)
+    return (S + getELFAddend(R)) & 0xFFFFFFFF;
+  return None;
+}
+
+static Optional<uint64_t> resolveARM(RelocationRef R, uint64_t S, int64_t A) {
+  if (R.getType() == ELF::R_ARM_ABS32) {
+    if ((int64_t)S < INT32_MIN || (int64_t)S > UINT32_MAX)
+      return None;
+    return static_cast<uint32_t>(S);
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveAVR(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case ELF::R_AVR_16:
+    return (S + getELFAddend(R)) & 0xFFFF;
+  case ELF::R_AVR_32:
+    return (S + getELFAddend(R)) & 0xFFFFFFFF;
+  }
+  return None;
+}
+
+static Optional<uint64_t> resolveLanai(RelocationRef R, uint64_t S, int64_t A) {
+  if (R.getType() == ELF::R_LANAI_32)
+    return (S + getELFAddend(R)) & 0xFFFFFFFF;
+  return None;
+}
+
+static Optional<uint64_t> resolveMips32(RelocationRef R, uint64_t S, int64_t A) {
+  // FIXME: Take in account implicit addends to get correct results.
+  uint32_t Rel = R.getType();
+  if (Rel == ELF::R_MIPS_32)
+    return (S + A) & 0xFFFFFFFF;
+  if (Rel == ELF::R_MIPS_TLS_DTPREL32)
+    return (S + A) & 0xFFFFFFFF;
+  return None;
+}
+
+static Optional<uint64_t> resolveSparc32(RelocationRef R, uint64_t S, int64_t A) {
+  uint32_t Rel = R.getType();
+  if (Rel == ELF::R_SPARC_32 || Rel == ELF::R_SPARC_UA32)
+    return S + getELFAddend(R);
+  return None;
+}
+
+static Optional<uint64_t> resolveHexagon(RelocationRef R, uint64_t S, int64_t A) {
+  if (R.getType() == ELF::R_HEX_32)
+    return S + getELFAddend(R);
+  return None;
+}
+
+static Optional<uint64_t> resolveCOFFX86(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case COFF::IMAGE_REL_I386_SECREL:
+  case COFF::IMAGE_REL_I386_DIR32:
+    return static_cast<uint32_t>(S + A);
+  default:
+    return None;
+  }
+}
+
+static Optional<uint64_t> resolveCOFFX86_64(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case COFF::IMAGE_REL_AMD64_SECREL:
+    return static_cast<uint32_t>(S + A);
+  case COFF::IMAGE_REL_AMD64_ADDR64:
+    return S + A;
+  default:
+    return None;
+  }
+}
+
+static Optional<uint64_t> resolveMachOX86_64(RelocationRef R, uint64_t S, int64_t A) {
+  if (R.getType() == MachO::X86_64_RELOC_UNSIGNED)
+    return S;
+  return None;
+}
+
+static Optional<uint64_t> resolveWasm32(RelocationRef R, uint64_t S, int64_t A) {
+  switch (R.getType()) {
+  case wasm::R_WASM_FUNCTION_INDEX_LEB:
+  case wasm::R_WASM_TABLE_INDEX_SLEB:
+  case wasm::R_WASM_TABLE_INDEX_I32:
+  case wasm::R_WASM_MEMORY_ADDR_LEB:
+  case wasm::R_WASM_MEMORY_ADDR_SLEB:
+  case wasm::R_WASM_MEMORY_ADDR_I32:
+  case wasm::R_WASM_TYPE_INDEX_LEB:
+  case wasm::R_WASM_GLOBAL_INDEX_LEB:
+  case wasm::R_WASM_FUNCTION_OFFSET_I32:
+  case wasm::R_WASM_SECTION_OFFSET_I32:
+  case wasm::R_WASM_EVENT_INDEX_LEB:
+    // For wasm section, its offset at 0 -- ignoring Value
+    return 0;
+  }
+  return None;
+}
+
+RelocationResolver getRelocationResolver(const ObjectFile &Obj) {
+  if (Obj.isCOFF())
+    return Obj.getBytesInAddress() == 4 ? resolveCOFFX86 : resolveCOFFX86_64;
+
+  if (Obj.isELF()) {
+    if (Obj.getBytesInAddress() == 8) {
+      switch (Obj.getArch()) {
+      case Triple::x86_64:
+        return resolveX86_64;
+      case Triple::aarch64:
+      case Triple::aarch64_be:
+        return resolveAarch64;
+      case Triple::bpfel:
+      case Triple::bpfeb:
+        return resolveBpf;
+      case Triple::mips64el:
+      case Triple::mips64:
+        return resolveMips64;
+      case Triple::ppc64le:
+      case Triple::ppc64:
+        return resolvePPC64;
+      case Triple::systemz:
+        return resolveSystemz;
+      case Triple::sparcv9:
+        return resolveSparc64;
+      case Triple::amdgcn:
+        return resolveAmdgpu;
+      default:
+        return nullptr;
+      }
+    }
+
+    // 32-bit object file
+    assert(Obj.getBytesInAddress() == 4 &&
+           "Invalid word size in object file");
+
+    switch (Obj.getArch()) {
+    case Triple::x86:
+      return resolveX86;
+    case Triple::ppc:
+      return resolvePPC32;
+    case Triple::arm:
+    case Triple::armeb:
+      return resolveARM;
+    case Triple::avr:
+      return resolveAVR;
+    case Triple::lanai:
+      return resolveLanai;
+    case Triple::mipsel:
+    case Triple::mips:
+      return resolveMips32;
+    case Triple::sparc:
+      return resolveSparc32;
+    case Triple::hexagon:
+      return resolveHexagon;
+    default:
+      return nullptr;
+    }
+  }
+
+  if (Obj.isMachO())
+    return Obj.getArch() == Triple::x86_64 ? resolveMachOX86_64 : nullptr;
+
+  if (Obj.isWasm())
+    return Obj.getArch() == Triple::wasm32 ? resolveWasm32 : nullptr;
+
+  return nullptr;
+}
+
+} // namespace object
+} // namespace llvm