[PATCH] D56031: [elfabi] Add support for reading dynamic symbols from binaries

[Armando Montanez via Phabricator via llvm-commits]

amontanez added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:195
+        reinterpret_cast<const Elf_GnuHash *>(TablePtr.get());
+    Count = std::max(Count, getDynSymtabSize<ELFT>(*Table));
+  }
----------------
jakehehrlich wrote:
> jakehehrlich wrote:
> > ELFT should be inferred here.
> i think rather than trying to be pedantic and get the largest index from both, you can just return right here.
I'm not fully certain why, but ELFT can't be inferred in this situation.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:243
+
+  if (RawSym.st_shndx == 0)
+    TargetSym.Undefined = true;
----------------
jakehehrlich wrote:
> Technically this gets a lot more complicated. You have a lot of special section indexes and there are there is this somewhat complicated thing for large symbol indexes that you don't need to implement now. You should check that the symbol index is below SHN_LORESERVE first. In final library form there is potential for this input from the disk to be wrong so you'll need to return an error anyhow.
> 
> Add a todo for the  other things like common, abs, large indexes, etc...
As per offline discussion, there's not really a case where any value of `st_shndx` other than SHN_UNDEF would make a symbol undefined.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56031