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

[Haowei Wu via Phabricator via llvm-commits]

haowei added a comment.

I rebased the code, it seems upset the Phabricator since it does not distinguish between rebase changes and actual changes.

In D93362#2486135 <https://reviews.llvm.org/D93362#2486135>, @jhenderson wrote:

> 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.

Do you mean I should also modify llvm-readobj so it use my `getDynSymtabSize` function to determine `.dynsym` size? I assume it currently has its own way to determine the number of dynamic symbols.



================
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++;
----------------
jhenderson wrote:
> Seems like this might enter an infinite loop, if there's a corrupt section without a 0 value at the end of the chain.
You are right, but what would be the best way to prevent that? I thought about to determine the max bucket capacity by calculating the differences between indexes in the bucket. However, it is not guaranteed that hashes will evenly distributed in the buckets. It also won't work if this is only one bucket.


================
Comment at: llvm/include/llvm/Object/ELF.h:584
+    if (Sec.sh_type == ELF::SHT_DYNSYM)
+      return Sec.sh_size / Sec.sh_entsize;
+  }
----------------
jhenderson wrote:
> 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.
I make it throw an error when the mod is not 0. Added brace since the return stmt takes 3 lines.


================
Comment at: llvm/include/llvm/Object/ELF.h:586
+  }
+  if (!SectionsOrError->size()) {
+    // Section headers are available but .dynsym header is not found.
----------------
grimar wrote:
> jhenderson wrote:
> > 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.
> You can use `SectionsOrError->empty()` for such checks.
Thanks for pointing out. I made a mistake here. Changed to empty() in latest diff.


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

https://reviews.llvm.org/D93362