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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:166
+  const Elf_Word *It =
+      reinterpret_cast<const Elf_Word *>(Table.buckets().end());
+  It += (BucketVal - Table.symndx);
----------------
Isn't this what values() is for?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:195
+        reinterpret_cast<const Elf_GnuHash *>(TablePtr.get());
+    Count = std::max(Count, getDynSymtabSize<ELFT>(*Table));
+  }
----------------
ELFT should be inferred here.


================
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:
> 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.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:198
+  // Search SYSV hash table to try to find the upper bound of dynsym.
+  if (Dyn.ElfHash.hasValue()) {
+    Expected<const uint8_t *> TablePtr = ElfFile.toMappedAddr(*Dyn.ElfHash);
----------------
I think you might consider adding ElfHash in a second patch instead.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:243
+
+  if (RawSym.st_shndx == 0)
+    TargetSym.Undefined = true;
----------------
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...


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:269
+  auto RawSym = DynSym.begin();
+  while (++RawSym < DynSym.end()) {
+    // If a symbol does not have global or weak binding, ignore it.
----------------
This should just be a range based for loop.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:271
+    // If a symbol does not have global or weak binding, ignore it.
+    uint8_t Binding = RawSym->st_info >> 4;
+    if (!(Binding == STB_GLOBAL || Binding == STB_WEAK))
----------------
There are macros to do this for I believe. Same with visibility.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56031