[PATCH] D83935: [llvm-readobj] - Move out the common code from printRelocations() methods.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6243-6248
+          if (R.Type == Relocation::RelType::Rel)
+            printRelocation(Obj, SecIndex, R.Rel, RelIndex, SymTab);
+          else if (R.Type == Relocation::RelType::Rela)
+            printRelocation(Obj, SecIndex, R.Rela, RelIndex, SymTab);
+          else
+            W.startLine() << W.hex(R.Relr) << "\n";
----------------
jhenderson wrote:
> This lambda (and the identical one in `printSectionHeaders`) seems a little unfortunate to me, if I'm honest. The `printRelocationsHelper` function switches on the section header type, whilst this switches on the relocation type, which can be inferred from the section header type. Would it not be simpler to call `printRelocation` inline? I'm not sure I see how the lambda improves things.
> The printRelocationsHelper function switches on the section header type, whilst this switches on the relocation type, which can be inferred from the section header type.

Inferring the relocation type is not that straightforward. It can be Rel/Relr depending on the `opts::RawRelr` switch, for example:

```
  case ELF::SHT_RELR:
  case ELF::SHT_ANDROID_RELR: {
    Elf_Relr_Range Relrs = unwrapOrError(this->FileName, Obj->relrs(&Sec));
    if (opts::RawRelr) {
      for (const Elf_Relr &R : Relrs)
        printRelrReloc(R);
      break;
    }
    std::vector<Elf_Rel> RelrRels =
        unwrapOrError(this->FileName, Obj->decode_relrs(Relrs));
    for (const Elf_Rel &R : RelrRels)
      printRelReloc(Obj, SecNdx, SymTab, R, ++RelNdx);
    break;
  }

```

And we want to avoid the duplication of the inferring logic probably, it is not that easy/convenient to share it I think.

> Would it not be simpler to call printRelocation inline?

It is possible, but we have different implementations of `printRelocation` for GNU/LLVM style.
And it is a template function that only handles Rel/Rela currently, but not Relr:

```
  template <class RelTy>
  void printRelocation(const ELFO *Obj, unsigned SecIndex, const RelTy &Rel,
                       unsigned RelIndex, const Elf_Shdr *SymTab);
```

So we can't jsut call/inline the existent logic. To be able to remove lambda, I see 2 ways:
1) Introduce 3 more virtual functions that will be overriden by GNU/LLVM dumpers:
`printRelReloc`, `printRelaReloc` and `printRelrReloc`.
2) Introduce a `Relocation` struct/union (like was done in the diff 3) and a single new virtual function.

I've updated the code to remove lambda and implemented (1). What do you think?



















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

https://reviews.llvm.org/D83935