[PATCH] D55629: [elfabi] Add support for reading DT_SONAME from binaries

Tue Dec 18 17:14:56 PST 2018

jakehehrlich added inline comments.

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-no-dt-strsz.test:19
+    Size:            32
+  - Name:            .dynstr
+    Type:            SHT_STRTAB
----------------
So yaml2obj actually does support dynamic symbols but I'm not sure that it supports .dynamic. grep for "DynamicSymbols" in .test and .yaml files in the code base. There should be a yaml2obj test that uses this feature. You might find proper support for .dynamic as well.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:80
+  Optional<uint64_t> StrTabSize;
+  for (auto &Entry : DynamicEntries) {
+    if (Entry.d_tag == DT_STRTAB)
----------------
I'd prefer to only do one loop over the dynamic entries; right now you do a seperate loop for each feature (here just .dynstr and the soname, but later other things as well). It's not a huge deal as there's a pretty small upper bound on how long these can be. A standard strategy is to traverse it once and put everything in an array indexed by the tag.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:56
+template <class ELFT>
+bool populateSoName(ELFStub &TargetStub,
+                    const typename ELFT::DynRange &DynamicEntries,
----------------
All these except readELFFile should be static and in ELFObjHandler.cpp and not in the header.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55629