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

[George Rimar via Phabricator via llvm-commits]

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,
----------------
grimar wrote:
> 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());
> ```
> 
> 
> 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); });
```

If the concern is about duplicating of this call, then it is eaily solvable by introducing
a new method to isolate it. I've added the `printRelocationsHelper` method back,
it is now incapsulates this `forEachRelocationDo` call.


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

https://reviews.llvm.org/D91530