Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries

[MaskRay]

Summary

This is lld-side change of D57939

[MaskRay]

wrap with unnamed namespace

[MaskRay]

.

[ruiu]

ELF/DWARF.cpp

111–112

I found this interface a bit awkward. Passing a DataRefImpl instance just to pass a single integer value seems odd to me.

I think my question is, is it that hard to read debug info? We don't necessarily have to use the library to read debug info, and if we can just read debug info with a small amount of code, that could be a better way. I don't know if that's the case, but that's something I want to investigate.

[dblaikie]

ELF/DWARF.cpp

111–112

If there's something awkward about LLVM's DWARF reading APIs, I'd love to see them improved (open to suggestions/requests/design discussions/patches) rather than forked/partially reimplemented in LLD, if at all reasonable/possible, for what it's worth.

DataRefImpl is a union of the integer and some other alternatives, by the looks of it - so I'm not sure I see this as being especially odd/awkward. I guess it's because some clients want to describe the parameter in one way, and some in another. If it's particularly hard to read here, there could be a convenience wrapper function that packs the parameter into a DataRefImpl/RelocationRef/etc?

[MaskRay]

ELF/DWARF.cpp

111–112

The slight weird thing here is that RelocationResolver<typename ELFT::Rela>::Resolve is the constant for every relocation involved. Putting it to every RelocAddrEntry is wasteful. I'll try improving this but for now I just want to get the SHT_RELA bug fixed.. D57939

[echristo]

ELF/DWARF.cpp

111–112

Seems reasonable. Do you have an idea of how you want to change things?

[MaskRay]

ELF/DWARF.cpp

111–112

1. Define another set of functions listing supported relocation types for each architecture. The function for x86_64 may look like: switch (R) { case R_X86_64_64: case R_X86_64_PC32: return true; default: return false; }. That would cause duplication but I cannot think of a better way if we also care about the performance (@dblaikie 's comment on D57949 made me think so)
2. Make heavier refactoring to the RelocAddrEntry call sites. Make RelocationResolver<typename ELFT::Rela>::Resolve a member of the class where RelocAddrEntry is used to resolve the relocation.

The two steps require some efforts but we probably should do that to make the code cleaner. Note, I believe the lld side DWARF code is simple enough and the amount of code added by this patch is also reasonable.

[echristo]

ELF/DWARF.cpp

58–71

This could probably use some explanatory comments here. I know what you mean by S and S+A but let's just have it explicitly called out. :)

111–112

This works for me.

[MaskRay]

Comment S and A

[echristo]

This is fine with me, Rui?

[ruiu]

I thought that I sent it before, but it's gone somewhere...

I wonder why you had to use template. Looks like you could just pass a plain function pointer. Is there any reason you didn't do that?

[MaskRay]

In D57940#1437293, @ruiu wrote:

I thought that I sent it before, but it's gone somewhere...

I wonder why you had to use template. Looks like you could just pass a plain function pointer. Is there any reason you didn't do that?

The reason I use template <class RelTy> struct RelocationResolver and make its partial specialization template <class ELFT> struct RelocationResolver<Elf_Rel_Impl<ELFT, false>> is because there are 4 ELFT types and each has the Rel variant and the Rela variant. If I de-template the struct, I'll have to define 8 functions.

[ruiu]

Not sure if I follow. Looks like you always instantiate the template using ELFT::Rela, so effectively you are assuming that all targets are using RELA (which is a wrong assumption by the way)? I think you need to dispatch using Config->IsRela.

[MaskRay]

dispatch using Config->IsRela

[ruiu]

LGTM

[MaskRay]

Dispatch on RelTy

[MaskRay]

In D57940#1438313, @ruiu wrote:

Not sure if I follow. Looks like you always instantiate the template using ELFT::Rela, so effectively you are assuming that all targets are using RELA (which is a wrong assumption by the way)? I think you need to dispatch using Config->IsRela.

Config->IsRela is the target default but the object file may have both SHT_REL and SHT_RELA (may not be realistic but then intention of AreRelocsRela is probably for this purpose). I've changed that.

[arichardson]

In D57940#1438813, @MaskRay wrote:

dispatch using Config->IsRela

The value of getAddend<ELFT>(Rel); now only seems to be in Ref.getRawDataRefImpl().p now. Shouldn't the rel case also be adding the implicit addend? I'm not sure I'm reading the code currently but it looks to me like REL no longer gets the addend added.

[MaskRay]

In D57940#1438854, @arichardson wrote:

In D57940#1438813, @MaskRay wrote:

dispatch using Config->IsRela

The value of getAddend<ELFT>(Rel); now only seems to be in Ref.getRawDataRefImpl().p now. Shouldn't the rel case also be adding the implicit addend? I'm not sure I'm reading the code currently but it looks to me like REL no longer gets the addend added.

For the REL case, the addend is supplied by the caller as the parameter A. A is supplied by llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp:getRelocatedValue.

ELF/DWARF.cpp

if (Sec->Name == ".debug_abbrev")
       AbbrevSection = toStringRef(Sec->data());
     else if (Sec->Name == ".debug_str")
       StrSection = toStringRef(Sec->data());
     else if (Sec->Name == ".debug_line_str")
       LineStringSection = toStringRef(Sec->data());
   }
 }
 
+namespace {
+template <class RelTy> struct Resolver {
+  static Optional<uint64_t> Resolve(object::RelocationRef Ref, uint64_t S,
+                                    int64_t A) {
+    return S + Ref.getRawDataRefImpl().p;
+  }
+};
+
+template <class ELFT> struct Resolver<Elf_Rel_Impl<ELFT, false>> {
+  static Optional<uint64_t> Resolve(object::RelocationRef Ref, uint64_t S,
+                                    int64_t A) {
+    return S + A;
+  }
+};
+} // namespace
+
 // Find if there is a relocation at Pos in Sec.  The code is a bit
 // more complicated than usual because we need to pass a section index
 // to llvm since it has no idea about InputSection.
 template <class ELFT>
 template <class RelTy>
 Optional<RelocAddrEntry>
 LLDDwarfObj<ELFT>::findAux(const InputSectionBase &Sec, uint64_t Pos,
                            ArrayRef<RelTy> Rels) const {
LLDDwarfObj<ELFT>::findAux(const InputSectionBase &Sec, uint64_t Pos,
   auto *DR = dyn_cast<Defined>(&File->getRelocTargetSym(Rel));
   if (!DR) {
     RelType Type = Rel.getType(Config->IsMips64EL);
     if (Type != Target->NoneRel)
       error(toString(File) + ": relocation " + lld::toString(Type) + " at 0x" +
             llvm::utohexstr(Rel.r_offset) + " has unsupported target");
     return None;
   }
-  uint64_t Val = DR->Value + getAddend<ELFT>(Rel);
+  uint64_t Val = DR->Value;
 
   // FIXME: We should be consistent about always adding the file
   // offset or not.
   if (DR->Section->Flags & ELF::SHF_ALLOC)
     Val += cast<InputSection>(DR->Section)->getOffsetInFile();
 
-  return RelocAddrEntry{SecIndex, Val};
+  DataRefImpl D;
+  D.p = getAddend<ELFT>(Rel);
+  return RelocAddrEntry{SecIndex, RelocationRef(D, nullptr),
+                        Resolver<typename ELFT::Rela>::Resolve, Val};
 }
 
 template <class ELFT>
 Optional<RelocAddrEntry> LLDDwarfObj<ELFT>::find(const llvm::DWARFSection &S,
                                                  uint64_t Pos) const {
   auto &Sec = static_cast<const LLDDWARFSection &>(S);
   if (Sec.Sec->AreRelocsRela)
     return findAux(*Sec.Sec, Pos, Sec.Sec->template relas<ELFT>());
   return findAux(*Sec.Sec, Pos, Sec.Sec->template rels<ELFT>());
 }
 
 template class elf::LLDDwarfObj<ELF32LE>;
 template class elf::LLDDwarfObj<ELF32BE>;
 template class elf::LLDDwarfObj<ELF64LE>;
 template class elf::LLDDwarfObj<ELF64BE>;