[PATCH] D69093: [llvm-objcopy] --add-symbol: fix crash if SHT_SYMTAB does not exist

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

LGTM with a 2 suggestions.



================
Comment at: test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:36
+# SEC2:      Index: 1
+# SEC2-NEXT: Name: .shstrtab
+# SEC2-NEXT: Type: SHT_STRTAB
----------------
MaskRay wrote:
> MaskRay wrote:
> > grimar wrote:
> > > ".shstrtab" != ".strtab"
> > Yes, we overload `.shstrtab` (section names only) for more purposes (section names+symbol names). I think this is fine, but it definitely deserves a comment.
> > 
> > (Similarly, clang reuse .strtab for both section names and symbol names.)
> > 
> > GNU objcopy does not allow such object files (I haven't looked into details what sections (.strtab or .symtab) it expects).
> > 
> > ```
> > % objcopy --add-symbol=abs1=1 t t2
> > objcopy: error: the input file 'a' has no sections
> > ```
> To expand a bit more on this topic.
> 
> For better test coverage, we should have a test that places .shstrtab before .strtab so that we can check the created .symtab picks .strtab as its sh_link field. But I don't know any way to reorder sections.
> 
> Since rL238073, clang no longer produces .shstrtab and uses a unified .strtab for both section names and symbol names. I cannot write something like
> 
>       if (Sec.Type == ELF::SHT_STRTAB && Obj.SectionNames != &Sec)
> 
> because such use cases will fail. Again, I cannot test it with existing yaml2obj functionality (I am not sure it is useful enough to make it possible to test such scenarios... probably not worth it)
I see, thanks for the explanation.


================
Comment at: test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:30
+
+## Check that we create both .strtab and .symtab.
+## We reuse the existing .shstrtab (section names) for symbol names.
----------------
So this should probably say "create both symbol table and string table".
I.e. use "string table" instead of ".strtab", because it is a bit confusing that
there is no any ".strtab" in the output.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1546
+    StringTableSection *StrTab = nullptr;
+    for (auto &Sec : Obj.sections())
+      if (Sec.Type == ELF::SHT_STRTAB) {
----------------
nit: I think you need to add curly bracers to wrap the 'for' body.

Perhaps something like the following?
```
for (auto &Sec : Obj.sections()) {
  if (Sec.Type == ELF::SHT_STRTAB)
    continue:
  StrTab = static_cast<StringTableSection *>(&Sec);
  // Prefer .strtab to .shstrtab.
  if (Obj.SectionNames != &Sec)
    break;
}
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69093