[PATCH] D49016: [llvm-objdump] Add dynamic section printing to private-headers option

Paul Semel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 17 07:17:22 PDT 2018


paulsemel added inline comments.


================
Comment at: include/llvm/ADT/STLExtras.h:1041-1045
+template <class T, std::size_t N>
+constexpr size_t size(const T (&array)[N]) noexcept {
+  return N;
+}
+
----------------
jhenderson wrote:
> Be careful that this doesn't get submitted with everything else...
Sure, I will wait for the other patch to be accepted, and then I'll rebase


================
Comment at: include/llvm/Object/ELF.h:451
+template <class ELFT>
+Expected<typename ELFT::DynRange> ELFFile<ELFT>::dynamicEntries() const {
+  const Elf_Dyn *Dyn = nullptr;
----------------
jhenderson wrote:
> jhenderson wrote:
> > I think this should use the dynamic program header, rather than the sections, because a runnable program doesn't need section headers (but it does not program headers). It'll be faster, because there are generally fewer program headers than sections too. Maybe it could fall back to using the section headers if there is no PT_DYNAMIC segment.
> > 
> > Are the functions in this file unit-tested at all? If so, we should add them. If not, we still might want to add some, but it would depend on how straightforward it is to do.
> I don't think you've addressed this comment: 
> >Are the functions in this file unit-tested at all? If so, we should add them. If not, we still might want to add some, but it would depend on how straightforward it is to do.
> 
> 
No there is basically two features unit tested in the libObject. Do you really want me to do one ?


================
Comment at: test/tools/llvm-objdump/private-headers-dynamic-section.test:27
+# CHECK:  VERSYM               0x00000000000003ec
+# CHECK:  RELACOUNT            0x0000000000000003
----------------
jhenderson wrote:
> Could you also add SHT_RELR cases here, please?
Are you talking about PLT REL entries ? I've added some anyway !


================
Comment at: tools/llvm-objdump/ELFDump.cpp:72-74
+    if (Phdr.p_type == ELF::PT_PHDR) {
+      VirtBase = Phdr.p_vaddr - Phdr.p_offset;
+      break;
----------------
jhenderson wrote:
> I don't think you've addressed my earlier comments about not using VirtBase, derived from a PT_PHDR?
I have eventually changed the way I'm getting the Dynamic String Table, to reuse at most as possible the libObject.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:83-89
+  for (const auto &Dyn : *DynamicEntriesOrError) {
+    if (Dyn.d_tag == ELF::DT_NULL)
+      continue;
+    StringRef Str;
+    for (size_t i = 0; i < size(DynamicTagsValues); ++i) {
+      if (DynamicTagsValues[i].Val == Dyn.d_tag) {
+        Str = StringRef(DynamicTagsValues[i].ToStr);
----------------
jhenderson wrote:
> This sort of area is a good example of why I think we should be reusing code from llvm-readobj. Please investigate this as a priority. In particular, I note that llvm-readobj does not have to loop through the whole set of tags to get the right string.
Well, I would just need to replace this to a switch actually


Repository:
  rL LLVM

https://reviews.llvm.org/D49016





More information about the llvm-commits mailing list