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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich accepted this revision.
jakehehrlich added a comment.
This revision is now accepted and ready to land.

Pending 1 nit, LGTM.



================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:195
+        reinterpret_cast<const Elf_GnuHash *>(TablePtr.get());
+    Count = std::max(Count, getDynSymtabSize<ELFT>(*Table));
+  }
----------------
amontanez wrote:
> 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.
I see why now, it is the "ELFT::..." You can think "::GnuHash" as acting like a function and the compiler would have to reverse that function. In fact there might not actually be a unique inverse.

There's an alternative, which is to use the underlying type that is paramterized by ELFT but this is *not* the intended surface of the ELFType library so you shouldn't do that. So this was correct as is I suppose.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:273-275
+    ELFSymbol Sym(*SymName);
+    populateSymbol<ELFT>(Sym, RawSym);
+    TargetStub.Symbols.insert(std::move(Sym));
----------------
This would all be nicer if populateSymbol was part of a constructor for ELFSymbol and then you could used `TargetStub.Symbols.emplace(*SymName, RawSym)` instead of these 3 lines.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56031