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

Thu Oct 17 02:57:35 PDT 2019

grimar 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
----------------
MaskRay wrote:
> 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.
> I have added an explicit Symbols: to ensure .symtab exists, even after D69041.

Yeah, that was my question. Do you plan to remove "Symbols:" and this call later or not.
I think it is not a problem to keep it, it makes things self-documented. So its OK as is.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D69093