[PATCH] D91530: [llvm-readobj] - Introduce `forEachRelocationDo` helper. NFCI.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3790
     printRelocHeaderFields<ELFT>(OS, Sec.sh_type);
-    this->printRelocationsHelper(Sec);
+    this->forEachRelocationDo(
+        Sec, opts::RawRelr,
----------------
MaskRay wrote:
> `this->printRelocationsHelper(Sec);` has been expanded into
> ```
> this->forEachRelocationDo(
>         Sec, opts::RawRelr,
>         [&](const Relocation<ELFT> &R, unsigned Ndx, const Elf_Shdr &Sec,
>             const Elf_Shdr *SymTab) { printReloc(R, Ndx, Sec, SymTab); },
>         [&](const Elf_Relr &R) { printRelrReloc(R); });
> ```
> multiple times. Isn't the previous version clearer?
> 
> Can you elaborate on the claim in the description?
> 
> > Our printStackSize implementation currently uses API like RelocationRef, object::symbol_iterator. It is not ideal as it doesn't allow to handle possible error conditions properly.
> Isn't the previous version clearer?

Do you compare with the original code or with the previous version of the patch?
Assuming that you compare with the original code: I am using this change in D91624. It allows to avoid
alot of duplication of error handling when iterating over relocations.

> Can you elaborate on the claim in the description?

Yeah. I think that API is just totally broken when used for invalid objects. See the description of D91624
about how `ELFObjectFile<ELFT>::symbol_end()` "works". In the test case for D91624 I've improved one of the messages and
now it provides more details about an error.

One more example:

```
  object::symbol_iterator RelocSym = Reloc.getSymbol();
  if (RelocSym != ElfObj.symbol_end()) {
```

The underlying implementation of `Reloc.getSymbol()` is:

```
symbol_iterator
ELFObjectFile<ELFT>::getRelocationSymbol(DataRefImpl Rel) const {
  uint32_t symbolIdx;
  const Elf_Shdr *sec = getRelSection(Rel);
  if (sec->sh_type == ELF::SHT_REL)
    symbolIdx = getRel(Rel)->getSymbol(EF.isMips64EL());
  else
    symbolIdx = getRela(Rel)->getSymbol(EF.isMips64EL());
  if (!symbolIdx)
    return symbol_end();

  // FIXME: error check symbolIdx
  DataRefImpl SymbolData;
  SymbolData.d.a = sec->sh_link;
  SymbolData.d.b = symbolIdx;
  return symbol_iterator(SymbolRef(SymbolData, this));
}
```

It just doesn't check `sec->sh_link` or `symbolIdx`

Another example is that in the `ELFObjectFile.h` file, the implementation of `ELFObjectFile<ELFT>` methods
has 5 `report_fatal_error` calls. They not just doesn't propogate them to user, but also hide real errors behind the
error code messages I think. E.g.:


```
  // Error check sh_link here so that getRelocationSymbol can just use it.
  auto SymSecOrErr = EF.getSection(RelSec->sh_link);
  if (!SymSecOrErr)
    report_fatal_error(errorToErrorCode(SymSecOrErr.takeError()).message());
```




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

https://reviews.llvm.org/D91530