[PATCH] D67343: [DebugInfo] Change object::RelocationResolver to return Expected<uint64_t>

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Relocation overflow check is probably not necessary. RelocationResolver is mainly used by DebugInfo (the use in llvm-readobj is very light) for local symbols. It is difficult to create any relocation overflow problems with meaningful tests. A somewhat clean approach to detect overflows:

  +static Expected<uint64_t> reportError(const char *type, const Twine &v,
  +                                      int64_t min, uint64_t max) {
  +  return createStringError(inconvertibleErrorCode(),
  +                           "relocation " + Twine(type) +
  +                               " out of range: " + Twine(v) + " is not in [" +
  +                               Twine(min) + ", " + Twine(max) + "]");
  +}
  +
  +static Expected<uint64_t> checkInt(const char *type, int64_t v, int n) {
  +  if (v == llvm::SignExtend64(v, n))
  +    return v;
  +  return reportError(type, Twine(v), minIntN(n), maxIntN(n));
  +}
  
  -  case ELF::R_X86_64_PC32:
  -    return S + getELFAddend(R) - R.getOffset();
  +  case ELF::R_X86_64_PC32: {
  +    uint64_t V =  S + getELFAddend(R) - R.getOffset();
  +    return checkInt("R_X86_64_PC32", V, 32);  // Passing the string literal is clumsy.
  +  }

Passing the string literal to `checkInt` is clumsy, but the lld way may be too heavyweight?

  StringRef llvm::object::getELFRelocationTypeName(uint32_t Machine,
                                                   uint32_t Type) {
    switch (Machine) {
    case ELF::EM_X86_64:
      switch (Type) {
  #include "llvm/BinaryFormat/ELFRelocs/x86_64.def"
      default:
        break;
      }
      break;

I'll probably abandon this if people don't think error reporting is useful here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67343/new/

https://reviews.llvm.org/D67343