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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:5
+## If .strtab exists but .symtab does not, create .symtab and set its sh_link to .strtab .
+# RUN: llvm-objcopy --remove-section=.symtab %t %t1
+# RUN: llvm-objcopy --add-symbol='abs1=1' %t1 %t2
----------------
grimar wrote:
> grimar wrote:
> > Will we want to remove this call when yaml2obj patch that supports omiting ".strtab" be landed?
> > If so - worth adding a TODOs, probably, I am not sure it is critical though.
> I meant ".symtab".
I have added an explicit `Symbols:` to ensure .symtab exists, even after D69041.

This should make the TODO unnecessary?

For similar reasons, I use an explicit `llvm-objcopy --remove-section=.symtab --remove-section=.strtab ` instead of llvm-strip to make it clear both sections are stripped below.


================
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
----------------
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
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69093