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

[Haowei Wu via Phabricator via llvm-commits]

haowei marked an inline comment as done.
haowei added a comment.

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

> Is it important that the .dynsym is used directly when .hash/.gnu.hash is available? At the moment, your test doesn't really show whether it is used or whether you go straight to the dynamic tags for details, I believe - to do that, you'd need to have one given bogus values for the size, and show that this is not sued. Additionally, you have no testing for your error messages, nor do you have testing that shows that the .gnu.hash is used above the .hash section.

I tried a few ways to generate corrupted the ELF file with yaml2obj but the results are not quite good. For the case to test the error message when the terminator in DT_GNU_HASH is not available, the loop at L540 in ELF.h actually ended before `It` hit `BufEnd`. The reason is that `.strtab` and `.shstrtab` are placed at the end of elf file automatically by yaml2obj and there will almost always be cases to make `(*It) & 1 != 0`, preventing the code from reach the error message. I think the only way would be placing .gnu.hash at the end of the ELF, and remove the terminator, but that seems to be not possible with yaml2obj.  For the case to test error message when `sh_size % sh_entsize is not 0`, I didn't find any good examples that I can manipulate the `sh_entsize` through yaml2obj. Do you have good suggestions to write unit tests for these cases?



================
Comment at: llvm/include/llvm/Object/ELF.h:602
+    return getDynSymtabSizeFromGnuHash<ELFT>(*Table,
+                                             (void *)this->Buf.bytes_end());
+  }
----------------
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*"


================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:3-4
+##  * Section headers are available
+##  * Section headers are stripped but .gnu.hash section is available.
+##  * Section headers are stripped but .hash section is available.
+
----------------
jhenderson wrote:
> Alternatively, refer to them by their dynamic tags:
> ```
> ##  * Section headers are stripped but there is a DT_GNU_HASH dynamic tag.
> ##  * Section headers are stripped but there is a DT_HASH dynamic tag.
> ```
> I think I prefer this form.
I will modify this in the next diff.


================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:10
+# RUN: yaml2obj --docnum=1 %s -o %tw.gnu.hash
+# RUN: llvm-strip --strip-sections %tw.gnu.hash
+# RUN: llvm-elfabi --elf %tw.gnu.hash --emit-tbe=- | FileCheck %s
----------------
jhenderson wrote:
> You don't need to call llvm-strip - yaml2obj can emit an object without the section header table, as pointed out in my earlier comment:
> 
> >>! In D93362#2486135, @jhenderson wrote:
> > 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.
> 
If I add

```
SectionHeaderTable:
  Sections:  []
```
it will report errors:
```
yaml2obj: error: section '.text' should be present in the 'Sections' or 'Excluded' lists
yaml2obj: error: section '.data' should be present in the 'Sections' or 'Excluded' lists
yaml2obj: error: section '.dynsym' should be present in the 'Sections' or 'Excluded' lists
yaml2obj: error: section '.hash' should be present in the 'Sections' or 'Excluded' lists
yaml2obj: error: section '.gnu.hash' should be present in the 'Sections' or 'Excluded' lists
yaml2obj: error: section '.dynstr' should be present in the 'Sections' or 'Excluded' lists
yaml2obj: error: section '.dynamic' should be present in the 'Sections' or 'Excluded' lists
yaml2obj: error: section '.strtab' should be present in the 'Sections' or 'Excluded' lists
yaml2obj: error: section '.shstrtab' should be present in the 'Sections' or 'Excluded' lists
```

If I try to use:

```
SectionHeaderTable:
  NoHeaders: true
```

Similar errors will be raised:

```
yaml2obj: error: excluded section referenced: '.text'  by symbol 'foo'
yaml2obj: error: excluded section referenced: '.data'  by symbol 'bar'
```

That's why I used llvm-strip instead of using this new yaml2obj feature. 


================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:140-141
+        Value: 0x400
+      - Tag:   DT_HASH
+        Value: 0x600
+      - Tag:   DT_NULL
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93362