[PATCH] D91533: [lib/Object] - Generalize the RelocationResolver API.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Object/RelocationResolver.cpp:42
 
-static uint64_t resolveX86_64(RelocationRef R, uint64_t S, uint64_t A) {
-  switch (R.getType()) {
+static uint64_t resolveX86_64(uint64_t Type, uint64_t Offset, uint64_t S,
+                              uint64_t LocData, int64_t Addend) {
----------------
MaskRay wrote:
> You have changed the addend parameter from uint64_t to int64_t. What's the motivation?
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;
```



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

https://reviews.llvm.org/D91533