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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:602
+    return getDynSymtabSizeFromGnuHash<ELFT>(*Table,
+                                             (void *)this->Buf.bytes_end());
+  }
----------------
haowei wrote:
> jhenderson wrote:
> > I don't think you need this cast?
> Without it, clang complains:
> 
> ```
> ELF.h:605:12: error: no matching function for call to 'getDynSymtabSizeFromGnuHash'
> ```
> If I recall correctly, bytes_end() returns "unsigned char*"
Actually, let's just change `getDynSymtabSizeFromGnuHash` to take a`uint8_t` for `BufEnd`. I think it's quite common in this code to work with bytes in that form.


================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:140-141
+        Value: 0x400
+      - Tag:   DT_HASH
+        Value: 0x600
+      - Tag:   DT_NULL
----------------
haowei wrote:
> jhenderson wrote:
> > By using yaml2obj macros, you can avoid having two nearly-identical YAML inputs. See the `-D` option in yaml2obj. This will also work with the "section headers present/not present options".
> The structure of DT_GNU_HASH is quite different from DT_HASH in yaml2obj if you compare L32 to L38 and L39-L51 . That's why I choose to put 2 elf YAML inputs instead of using marco. If you feel keeping these 2 sections in place and only reference one in the .dynamic section with macros is an acceptable approach, I can do that. Since I need to add a test case that section headers are available without any hash tables, I still have to put more than 1 YAML input in this test file.
I'm just suggesting using macros in the dynamic table. You can also use macros to avoid need for any extra YAML, I think. Just change the DT_* value to something arbitrary/omit individual sections from the section header table using the `Exclude` option of the `SectionHeaderTable` key etc depending on the specific example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93362