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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
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:
> 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)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69093