[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: tools/llvm-objcopy/ELF/Object.cpp:308-309
 uint32_t StringTableSection::findIndex(StringRef Name) const {
+  if (Name.empty())
+    return 0;
   return StrTabBuilder.getOffset(Name);
----------------
grimar wrote:
> jhenderson wrote:
> > 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.
> I am not sure I understand. `StrTabBuilder` is an instance of `llvm/MC/StringTableBuilder` class. 
> It is used for `enum Kind { ELF, WinCOFF, MachO, RAW, DWARF };` string tables.
> 
> Now it works fine for adding the empty string or any other strings.
> Empty string's offset is not guaranteed to have a zero offset after the optimization and it seems
> fine to me. Why would string builder need to know we need to have a zero offset for no-string (empty) ELF only case?
> 
> 
Ah, I think I see a problem. I imagined that the ELF kind of StringTableBuilder would add a null string to its string mapping during initialisation. This would mean that its `getOffset` method could be called with empty string without having to explicitly add an empty string. Adding it explicitly (e.g. in the constructor) though could cause optimization to move the initial empty string to another location (which would be wrong).

Basically, I find it strange that we have to guard against an empty string lookup, when we know that there is always such a string in the StringTableBuilder instance for ELF kinds (i.e. I don't like the need for the check, just to prevent an assertion for a string not being found).


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

https://reviews.llvm.org/D59496