[PATCH] D49369: [llvm-readobj] - Teach tool to dump objects with >= SHN_LORESERVE of sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added inline comments.
This revision now requires changes to proceed.


================
Comment at: test/tools/llvm-readobj/many-sections.s:1
+## many-sections.elf-x86_64 is a file that generated to simulate
+## an object with more than ~65k sections. When ELF object
----------------
Thanks for the descriptive comment! I've got a few grammar nits though.

that generated -> that was


================
Comment at: test/tools/llvm-readobj/many-sections.s:2
+## many-sections.elf-x86_64 is a file that generated to simulate
+## an object with more than ~65k sections. When ELF object
+## has SHN_LORESERVE (0xff00) or more of sections, it's e_shnum field
----------------
When ELF -> When an ELF


================
Comment at: test/tools/llvm-readobj/many-sections.s:3
+## an object with more than ~65k sections. When ELF object
+## has SHN_LORESERVE (0xff00) or more of sections, it's e_shnum field
+## should be zero and sh_size of the section header at index 0 used
----------------
more of sections -> more sections
it's -> its


================
Comment at: test/tools/llvm-readobj/many-sections.s:4
+## has SHN_LORESERVE (0xff00) or more of sections, it's e_shnum field
+## should be zero and sh_size of the section header at index 0 used
+## to store the value. If section name string table section index is
----------------
used -> is used


================
Comment at: test/tools/llvm-readobj/many-sections.s:5
+## should be zero and sh_size of the section header at index 0 used
+## to store the value. If section name string table section index is
+## greater than or equal to SHN_LORESERVE, then e_shstrndx field
----------------
If section... -> If the section


================
Comment at: test/tools/llvm-readobj/many-sections.s:8
+## should have the value of SHN_XINDEX and sh_link of the section header
+## at index 0 used to store the value.
+##
----------------
used -> is used


================
Comment at: test/tools/llvm-readobj/many-sections.s:10-12
+## many-sections.elf-x86_64 has little amount of sections to save disk
+## space, but its e_shnum, e_shstrndx, sh_size and sh_link fields are set
+## in according to above description, so that we can test the dumper.
----------------
has little amount of sections -> has few sections
set in according to above -> set according to the above


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:2474
+template <class ELFT>
+static std::string getSectionHeadersNumString(const ELFFile<ELFT> *Obj) {
+  const typename ELFT::Ehdr *ElfHeader = Obj->getHeader();
----------------
It may be worth giving a comment here and on the next function briefly explaining the rules, similar to what you did in the test.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:2476
+  const typename ELFT::Ehdr *ElfHeader = Obj->getHeader();
+  if (ElfHeader->e_shnum)
+    return to_string(ElfHeader->e_shnum);
----------------
Personally, I'd be explicit here, and say `ElfHeader->e_shnum > 0`, but I don't mind that much, if you prefer it this way.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:2478-2479
+    return to_string(ElfHeader->e_shnum);
+  const typename ELFFile<ELFT>::Elf_Shdr *Sec = unwrapOrError(Obj->getSection(0));
+  return "0 (" + to_string(Sec->sh_size) + ")";
+}
----------------
What about the situation where a file has no section headers? This will crash, I think. Please fix and add a test for this case. I think if e_shnum is zero, you need to check e_shoff to see if it has a non-zero value to decide if there are section headers.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:2487
+    return to_string(ElfHeader->e_shstrndx);
+  const typename ELFFile<ELFT>::Elf_Shdr *Sec = unwrapOrError(Obj->getSection(0));
+  return to_string(ElfHeader->e_shstrndx) + " (" + to_string(Sec->sh_link) + ")";
----------------
Again, what happens if there are no section headers here? In this case, the object is malformed, and we should probably report an error or warning, but still write the actual value in the field.


https://reviews.llvm.org/D49369