[PATCH] D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The testing for this is going to be somewhat similar to various llvm-readobj examples, which use yaml2obj to generate an ELF object with the required properties. For example, llvm\test\tools\llvm-readobj\ELF\dyn-symbols-size-from-hash-table.test contains testing for the llvm-readobj handling of dynamic symbols when there are no section headers. There are actually further improvements that can be made beyond that. For example, yaml2obj has recently gained the ability to not emit the section header table, rather than needing to use llvm-strip to remove it. See llvm\test\tools\yaml2obj\ELF\section-headers.yaml for an example.



================
Comment at: llvm/include/llvm/Object/ELF.h:562
+  }
+  LastSymIdx += BucketVal;
+  const Elf_Word *It =
----------------
By my reading of this code, there is no need for separate `LastSymIdx` and `BucketVal` variables. Can you get rid of one?


================
Comment at: llvm/include/llvm/Object/ELF.h:566
+  // Locate the end of the chain to find the last symbol index.
+  while ((*It & 1) == 0) {
+    LastSymIdx++;
----------------
Seems like this might enter an infinite loop, if there's a corrupt section without a 0 value at the end of the chain.


================
Comment at: llvm/include/llvm/Object/ELF.h:584
+    if (Sec.sh_type == ELF::SHT_DYNSYM)
+      return Sec.sh_size / Sec.sh_entsize;
+  }
----------------
This should probably return an error if `Sec.sh_size % Sec.sh_entsize != 0`. If an earlier piece of code makes it impossible to hit that case, then it probably still deserves an assertion at least.


================
Comment at: llvm/include/llvm/Object/ELF.h:586
+  }
+  if (!SectionsOrError->size()) {
+    // Section headers are available but .dynsym header is not found.
----------------
This looks inverted to me?

Also add a blank line after each of the if statemetns and for loops, to better breka this up into logical chunks.


================
Comment at: llvm/include/llvm/Object/ELF.h:605
+    case ELF::DT_GNU_HASH:
+      ElfGnuHash = Entry.d_un.d_ptr;
+    }
----------------
Although it's not strictly needed, I'd add a `break` here too, to ensure there's no bug risk if this switch were to ever be extended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93362