Page MenuHomePhabricator

[lib/Object] - Generalize the RelocationResolver API.
ClosedPublic

Authored by grimar on Mon, Nov 16, 5:18 AM.

Details

Summary

This changes the RelocationResolver to not use RelocationRef anymore:

using RelocationResolver = uint64_t (*)(uint64_t Type, uint64_t Offset,
                                        uint64_t S, uint64_t LocData,
                                        int64_t Addend);

It allows to reuse the RelocationResolver from the code
that doesn't want to deal with RelocationRef related API.

I am going to use it in llvm-readobj. See the description
of D91530 for more details.

Diff Detail

Event Timeline

grimar created this revision.Mon, Nov 16, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 16, 5:18 AM
grimar requested review of this revision.Mon, Nov 16, 5:18 AM
MaskRay added a comment.EditedMon, Nov 16, 8:45 PM

Seems reasonable. RelocationRef may be awkward to use just because it needs an OwningObject.

Generalize the RelocationResolver API

The description should capture the main point of the patch: change the following type to not use RelocationRef:

using RelocationResolver = uint64_t (*)(uint64_t Type, uint64_t Offset,
                                        uint64_t S, uint64_t LocData,
                                        int64_t Addend);
llvm/lib/Object/RelocationResolver.cpp
42–44

You have changed the addend parameter from uint64_t to int64_t. What's the motivation?

grimar edited the summary of this revision. (Show Details)Mon, Nov 16, 11:48 PM
grimar added inline comments.
llvm/lib/Object/RelocationResolver.cpp
42–44

Actually I am not. See: previously the code called getELFAddend which returns int64_t.
The implementation is still the same:

template <class ELFT>
Expected<int64_t>
ELFObjectFile<ELFT>::getRelocationAddend(DataRefImpl Rel) const {
  if (getRelSection(Rel)->sh_type != ELF::SHT_RELA)
    return createError("Section is not SHT_RELA");
  return (int64_t)getRela(Rel)->r_addend;
}

The argument was of type uint64_t and was called A. But in fact it was not r_addend,
it is the value of data at relocation location (which as we know might contain the addend, e.g on x86,
see how this the API is used by ELFDumper.cpp).
So, I've just renamed uint64_t A -> uint64_t LocData and added int64_t Addend. I haven't changed the type for it.

It just seems that the code here in this file not super clean/consistent. E.g. see:

static uint64_t resolveSparc32(RelocationRef R, uint64_t S, uint64_t A) {
  uint32_t Rel = R.getType();
  if (Rel == ELF::R_SPARC_32 || Rel == ELF::R_SPARC_UA32)
    return S + getELFAddend(R);
  return A;
}

or:

static uint64_t resolveX86_64(RelocationRef R, uint64_t S, uint64_t A) {
 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);

I.e. It might use both getELFAddend(R) and A and seems might require some cleanup. I did not want to
do any functional changes in this patch though. I believe my change fully preserves the original functionality
and just allows to reuse the API.

Also, as far I understand, we need to have both LocData and Addend. E.g. the current implementation for RISCV
uses both of them:

static uint64_t resolveRISCV(uint64_t Type, uint64_t Offset, uint64_t S,
                             uint64_t LocData, int64_t Addend) {
  int64_t RA = Addend;
  int64_t A = LocData;
.....
    return (A & 0xC0) | ((S + RA) & 0x3F);
  case ELF::R_RISCV_SUB6:
    return (A & 0xC0) | (((A & 0x3F) - (S + RA)) & 0x3F);
  case ELF::R_RISCV_ADD8:
    return (A + (S + RA)) & 0xFF;
  case ELF::R_RISCV_SUB8:
    return (A - (S + RA)) & 0xFF;

Yes that some RISC-V relocations need both the addend and the implicit addend (LocData; I wonder whether we can improve the name but I don't have a good suggestion)

It just seems that the code here in this file not super clean/consistent. E.g. see:

Yes. In addition, the file is under-tested. It is used for DWARF/llvm-xray/.eh_frame but many relocation types don't have good way for testing.

jhenderson accepted this revision.Wed, Nov 18, 1:19 AM

All looks good to me, but probably want @MaskRay's input too.

llvm/lib/Object/RelocationResolver.cpp
270

In all these functions, it might be clearer if you commented out/deleted the name of the unused parameters.

This revision is now accepted and ready to land.Wed, Nov 18, 1:19 AM
grimar planned changes to this revision.Wed, Nov 18, 3:39 AM

It turns out that LLD also uses this API when works with DWARF. It doesn't compile now and I am investigating how to fix it.

grimar updated this revision to Diff 306055.Wed, Nov 18, 4:49 AM
  • Updated implementation to make LLD compilable.
  • Addressed review comments.

Note: The way how LLD works with relocation resolver API
is a bit tricky. Main logic is located in LLDDwarfObj<ELFT>::findAux
and LLDRelocationResolver::resolve functions:

https://github.com/llvm/llvm-project/blob/master/lld/ELF/DWARF.cpp#L78
https://github.com/llvm/llvm-project/blob/master/lld/ELF/DWARF.cpp#L126

the idea is that it creates the RelocationRef with no owning object
and sets the DataRefImpl.d field either to 0 (for Rel) or to r_addend (for Rela).
Then the DWARFDataExtractor::getRelocatedValue also passes the piece of object data
which contains the addend for Rel objects.

After that, LLD provides 2 resolve functions that do either s + a or s + ref.getRawDataRefImpl().p.
They don't use relocation types and offsets because it is assumed that the computation is
always (S + A). No owning object is requred in this scheme, so it worked before.

The tricky part is that the RelocationRef with no owning object is a specific corner case.
Without an owning object it is not possible to call RelocationRef::getType(),
RelocationRef::getOffset() etc methods.

I had to update the resolveRelocation method to support the LLD specific way of using it.

This revision is now accepted and ready to land.Wed, Nov 18, 4:49 AM
grimar requested review of this revision.Wed, Nov 18, 4:49 AM
grimar updated this revision to Diff 306057.Wed, Nov 18, 4:54 AM
  • Rebased correctly.
MaskRay accepted this revision.Wed, Nov 18, 5:31 PM
MaskRay added inline comments.
lld/ELF/DWARF.cpp
92–96

"stored in the field to be relocated" => "extracted from the relocated location"

http://www.sco.com/developers/gabi/latest/ch4.reloc.html has a similar wording.

This revision is now accepted and ready to land.Wed, Nov 18, 5:31 PM

Optional suggestion: I wonder whether RelocatedValue may be better than LocData

jhenderson added inline comments.Thu, Nov 19, 1:10 AM
llvm/lib/Object/RelocationResolver.cpp
754
757

Is it simply not possible to provide Type and Offset information here? Otherwise, this code seems a little fragile to LLD deciding to try to use those fields for something.

grimar marked an inline comment as done.Thu, Nov 19, 1:16 AM
grimar added inline comments.
llvm/lib/Object/RelocationResolver.cpp
757

Yes, it is not possible without an owning object :(

inline uint64_t RelocationRef::getOffset() const {
  return OwningObject->getRelocationOffset(RelocationPimpl);
}

inline uint64_t RelocationRef::getType() const {
  return OwningObject->getRelocationType(RelocationPimpl);
}
jhenderson added inline comments.Thu, Nov 19, 1:23 AM
llvm/lib/Object/RelocationResolver.cpp
757

I think we need some sort of note added to the LLD resolver then, indicating that the values will always be 0 then (possibly could be an assertion?), because the information isn't available. That would at least hint to future developers to not try to use them.

grimar marked 3 inline comments as done.Thu, Nov 19, 1:27 AM
grimar added inline comments.
llvm/lib/Object/RelocationResolver.cpp
757

The type and offset fields are commented out currently in lld/ELF/DWARF.cpp:

static uint64_t resolve(uint64_t /*type*/, uint64_t /*offset*/, uint64_t s,
                        uint64_t /*locData*/, int64_t addend) {
  return s + addend;
}

Do you want me to add an explicit comment about them or uncomment and add an assertion?

jhenderson added inline comments.Thu, Nov 19, 1:44 AM
llvm/lib/Object/RelocationResolver.cpp
757

Either an explicit comment or assertion would be fine by me. Probably comment, because the assertion will need explaining anyway.

grimar updated this revision to Diff 306342.Thu, Nov 19, 2:00 AM
  • Addressed review comments.
This revision was automatically updated to reflect the committed changes.