[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#2500294 <https://reviews.llvm.org/D93362#2500294>, @grimar wrote:

> In D93362#2499834 <https://reviews.llvm.org/D93362#2499834>, @haowei wrote:
>
>> 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?
>
> Can you elaborate what the problem is? There are many examples in both `yaml2obj` and `llvm-readelf` test suites that are using the `EntSize` key to set the `sh_entsize`.

I thought EntSize was automatically generated and could not be modified. Thanks for the note, it solved my issue.



================
Comment at: llvm/include/llvm/Object/ELF.h:541-542
+  while (It <= BufEnd && (*It & 1) == 0) {
+    LastSymIdx++;
+    It++;
+  }
----------------
jhenderson wrote:
> LLVM usually uses preincrement (i.e. `++LastSymIdx` etc).
Changed to preincrement


================
Comment at: llvm/include/llvm/Object/ELF.h:566
+            object_error::parse_failed,
+            "malformed ELF, sh_size % sh_entsize is not 0");
+      }
----------------
jhenderson wrote:
> Similar to above, but also I'd include the sh_size and sh_entsize values in this message, e.g. like the following pseudo code.
> 
> `SecName + " section has sh_size (" + Sec.sh_size + ") % sh_entsize (" + Sec.sh_size + ") that is not 0"`
The actual SecName is a bit complicated to get and when this error happens it also means the ELF file is probably corrupted, so I put SHT_DYNSYM here instead. Let me know if raw_string_ostream is not the best approach to build string here.


================
Comment at: llvm/include/llvm/Object/ELF.h:602
+    return getDynSymtabSizeFromGnuHash<ELFT>(*Table,
+                                             (void *)this->Buf.bytes_end());
+  }
----------------
jhenderson wrote:
> 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.
Do you mean `uint8_t *`?  Actually the problem is I forgot the `const` modifier. After adding this, it no longer need casting.


================
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
----------------
grimar wrote:
> haowei wrote:
> > 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. 
> Using of `Sections:  []` is not what you want, it doesn't remove the section headers. It can be used to create a
> section headers table with a null section only:
> 
> ```
> SectionHeaderTable:
>   Sections: []
>   Excluded:
>     - Name: .strtab
>     - Name: .shstrtab
> ```
> 
> Though, you don`t actually need it, as you can write just the following to achieve the same:
> 
> ```
> SectionHeaderTable:
>   Excluded:
>     - Name: .strtab
>     - Name: .shstrtab
> ```
> 
> So, to exclude the section header table you need to use `NoHeaders: true`.
> The errors you see:
> 
> ```
> yaml2obj: error: excluded section referenced: '.text'  by symbol 'foo'
> yaml2obj: error: excluded section referenced: '.data'  by symbol 'bar'
> ```
> 
> are because your dynamic symbols specify sections:
> 
> ```
> DynamicSymbols:
>   - Name:    foo
>     Type:    STT_FUNC
>     Section: .text
>     Value:   0x100
>     Binding: 1
>   - Name:    bar
>     Type:    STT_OBJECT
>     Section: .data
>     Value:   0x200
>     Binding: 1
> ```
> 
> Can't you just remove `Section: .text` and `Section: .data`?
Removing `Section: .text` and `Section: .data` will causing llvm-elfabi's output indicate the symbols are undefined (which is true). I make it pointing something else. 

I think using the llvm-strip here is better. I need to do a lot of macro hacks to make this test to use a single ELF yaml input, while I can use a single one with llvm-strip. It looks cleaner.


================
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:
> 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.
I see. I use macros in .dynamic and this approach only requires a single YAML input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93362