[PATCH] D59496: [llvm-objcopy] - Fix a st_name of the first symbol table entry.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 19 03:37:03 PDT 2019
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
More information about the llvm-commits
mailing list