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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D59496#1433351 <https://reviews.llvm.org/D59496#1433351>, @jhenderson wrote:

> Perhaps you should add a test for other empty-named symbols?


I added a test for a section symbol.
Since we have 2 noticeable possible cases here: null symbols and all others, I think it should be enough,
probably no need to test all kinds of symbols that can have empty names.



================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:301
+void StringTableSection::addString(StringRef Name) {
+  assert(!Name.empty());
+  StrTabBuilder.add(Name);
----------------
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?


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:476
   for (auto &Sym : Symbols) {
-    Sym->NameIndex = SymbolNames->findIndex(Sym->Name);
+    Sym->NameIndex = Sym->Name.empty() ? 0 : SymbolNames->findIndex(Sym->Name);
     if (Sym->Binding == STB_LOCAL)
----------------
jakehehrlich wrote:
> Can't findIndex just return zero in such a case?
OK


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:498
   // size before layout is decided.
+  // Ignore the symbols with empty names, e.g special first symbol entry,
+  // some sections symbols etc.
----------------
jhenderson wrote:
> e.g special first symbol entry -> e.g. the null symbol entry
> 
> for consistency with our wording elsewhere, I believe.
Thanks. This comment was moved.


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

https://reviews.llvm.org/D59496