[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:529
+static Expected<uint64_t>
+getDynSymtabSizeFromGnuHash(const typename ELFT::GnuHash &Table, const void *BufEnd) {
+  using Elf_Word = typename ELFT::Word;
----------------
Don't forget to clang-format this and other code again with your latest changes.


================
Comment at: llvm/include/llvm/Object/ELF.h:566
+            object_error::parse_failed,
+            "malformed ELF, sh_size % sh_entsize is not 0");
+      }
----------------
haowei wrote:
> 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.
You can use `Twine` instead, I think:

```
return createStringError(object_error::parse_failed, "SHT_DYNSYM section has sh_size ("
  + Twine(Sec.sh_size) + ") % sh_entsize (" + Sec.sh_entsize + ") that is not 0");
```

You might need to do `.str()` on the end of the message too.


================
Comment at: llvm/include/llvm/Object/ELF.h:602
+    return getDynSymtabSizeFromGnuHash<ELFT>(*Table,
+                                             (void *)this->Buf.bytes_end());
+  }
----------------
haowei wrote:
> 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.
Yeah, I meant `uint8_t *`, but adding the `const` is probably enough.


================
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
----------------
haowei wrote:
> 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.
I'm not going to argue this strongly, but it's worth noting that llvm-objcopy and llvm-strip operations can have minor side effects, whereas with yaml2obj you have more precise control over what the output actually is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93362