[PATCH] D59496: [llvm-objcopy] - Fix a st_name of the first symbol table entry.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/symbol-empty-name.test:31
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
----------------
Nit: Could you reduce the number of spaces between the tag and the value, please, here and below? I'm finding it a little hard to follow what maps to what! Keep them lined up within the block of tags, but remove all but one space for the longest tag name like this:

```
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_EXEC
  Machine: EM_X86_64
```


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:301
+void StringTableSection::addString(StringRef Name) {
+  assert(!Name.empty());
+  StrTabBuilder.add(Name);
----------------
grimar wrote:
> jakehehrlich wrote:
> > Seems like that shouldn't be an error
> Does not seem to make sense to add an empty string to a string builder too though.
> (while `StringTableSection::findIndex` explicitly returns 0 for an empty string case)
> 
> Are you ok with the new code?
Personally, I quite like the behaviour in the latest version of the patch.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:308-309
 uint32_t StringTableSection::findIndex(StringRef Name) const {
+  if (Name.empty())
+    return 0;
   return StrTabBuilder.getOffset(Name);
----------------
I'm thinking that the `StrTabBuilder` class should actually store an empty string explicitly, rather than have an implicit one as it currently does. That would allow you to do `findIndex` with an empty string, with no problem.


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

https://reviews.llvm.org/D59496