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

[Haowei Wu via Phabricator via llvm-commits]

haowei added a comment.

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

> What others said - there are six cases we should care about:
>
> 1. Section headers are stripped and the ELF has no hash/gnu_hash dynamic tag - this can be treated as an error, I think since the ELF gABI requires a hash table.
> 2. Section headers are stripped, but there is a DT_HASH
> 3. Section headers are stripped, but there is a DT_GNU_HASH
> 4. Section headers present but there is no .hash or .gnu.hash section - this might be treated as an error, I think since the ELF gABI requires a hash table.
> 5. Section headers present with .hash section
> 6. Section headers present with .gnu.hash section
>
> I think all of the tools should handle all of these situations (llvm-readelf, llvm-objdump, llvm-elfabi, ...) so it definitely seems like this should be part of the Object library.

So actually, the ELF stub generated by elfabi does not have DT_HASH or DT_GNU_HASH. We have test it they are not required in dynamic linking. It does have section headers. So in the new diff, I moved this function into `llvm/include/llvm/Object/ELF.h`, and it fallback to parse .gnu.hash and .hash when section headers are not available. It returns 0 if none is found and return an error only if parsing error happens.

What would be best way to test this piece of code? Shall I put a few ELF files in the test directory with section headers, .hash, .gnu.hash stripped and run elfabi on them?



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:453
+  for (const Elf_Shdr &Sec : *SectionsOrError) {
+    if (Sec.sh_type == ELF::SHT_DYNSYM)
+      return Sec.sh_size / Sec.sh_entsize;
----------------
pcc wrote:
> This code won't find the section header if section headers have been stripped. You'll need to bring back the code that you deleted and extend it to infer the number of symbols from the `.hash` section. In general you can expect a binary to have at least one of `.hash` or `.gnu.hash` if it has a symbol table because there isn't enough information to interpret the dynamic symbol table without a hash table and without reading the section headers.
I see. The reason we want to change is that we found a so file with empty dynsym and .gnu.hash caused assertion error in 

```
llvm-project/llvm/include/llvm/Object/ELFTypes.h:542: ArrayRef<llvm::object::Elf_GnuHash_Impl::Elf_Word> llvm::object::Elf_GnuHash_Impl<llvm::object::ELFType<llvm::support::little, true> >::values(unsigned int) const [ELFT = llvm::object::ELFType<llvm::support::little, true>]: Assertion `DynamicSymCount >= symndx' failed.
```

Since elfabi is primarily used during linking, it is expected the input will have section headers so that why I made the change. In the new diff, I put this function into ELF.h and make it read section headers first and use .gnu.hash and .hash as backup.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:465
-  return LastSymIdx + 1;
-}
-
----------------
grimar wrote:
> FTR, I think we should move this somewhere to libObject. This will be useful for llvm-readelf/obj which is currently supports inferring symbol table size from `.hash`, but not from `.gnu.hash.
I see. In the new diff, I put it into `llvm/include/llvm/Object/ELF.h`, is it a good place for it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93362