[PATCH] D55852: [elfabi] Add support for reading DT_NEEDED from binaries

Tue Dec 18 17:06:38 PST 2018

jakehehrlich added a comment.

Other than me realizing I made a mistake in previous reviews and a minor little issue (NeededLibCount), this LGTM.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:122
+template <class ELFT>
+int populateNeededLibs(ELFStub &TargetStub,
+                       const typename ELFT::DynRange &DynamicEntries,
----------------
You never use the NeededLibCount. Why do you have it?

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:31
 Expected<std::unique_ptr<ELFStub>>
 buildStub(const object::ELFObjectFile<ELFT> &ElfObj);

----------------
ditto (below)

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:37
 template <class ELFT>
 Error populateArch(ELFStub &TargetStub, const typename ELFT::Ehdr &Header);

----------------
ditto (below)

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:45
 template <class ELFT>
 Expected<StringRef> getDynStr(const typename ELFT::DynRange &DynamicEntries,
                               const uint8_t *ObjBuffer);
----------------
ditto (below)

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:56
 template <class ELFT>
 bool populateSoName(ELFStub &TargetStub,
                     const typename ELFT::DynRange &DynamicEntries,
----------------
Just realized something I should have caught in previous reviews. You don't need these in the headers. You can make them static template functions in just the file they're used in. That reduces .o size and improves the compilers ability to perform optimizations.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:69
+template <class ELFT>
+int populateNeededLibs(ELFStub &TargetStub,
+                       const typename ELFT::DynRange &DynamicEntries,
----------------
Ditto here.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55852