[PATCH] D93209: [libObject, llvm-readobj] - Reimplement `ELFFile<ELFT>::getEntry`.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/invalid-symbol-table-size.test:6
 # RUN: yaml2obj -DBITS=32 -DSIZE=33 -DDYNSIZE=32 %s -o %t.32-bit.o
-# RUN: not llvm-nm %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 -DINDEX=2 %s
+# RUN: not --crash llvm-nm %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 -DINDEX=2 %s
 # RUN: yaml2obj -DBITS=64 -DSIZE=49 -DDYNSIZE=48 %s -o %t.64-bit.o
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > Seems to me like this change is a regression on the old behaviour? Same with llvm-objdump and llvm-size. Please don't land it with this.
> > This is because of the following code in `llvm/Object/ELFObjectFile.h`
> > 
> > ```
> >   const Elf_Sym *getSymbol(DataRefImpl Sym) const {
> >     auto Ret = EF.template getEntry<Elf_Sym>(Sym.d.a, Sym.d.b);
> >     if (!Ret)
> >       report_fatal_error(Ret.takeError());
> >     return *Ret;
> >   }
> > ```
> > 
> > Previously `getEntry<Elf_Sym>` ignored this error
> > Now it started to call `report_fatal_error` right here.
> > 
> > Ideally, we should never call `report_fatal_error`. I am also not sure I can call it a regression,
> > because the code had this call before my change too. I.e. I believe it might report `LLVM ERROR`
> > for another broken input already. It is just a luck that it "worked".
> > And the root of the issue is in `Elf_Sym *getSymbol`, which should return `Expected<>` I think.
> > 
> > Do you want me to update the `getSymbol` first?
> The way I see it, before we weren't getting crashes in the test and now we are, due to the reworking of the function, so it's a regression, at least for the cases that are tested. I think fixing `getSymbol` may be a prerequisite of this change then.
I've prepared the patch: D93297


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

https://reviews.llvm.org/D93209