[PATCH] D82935: [llvm-objcopy] Fix crash when removing symbol table at same time as adding a symbol

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I don't see a benefit in having two different places where the symbol table can be created. I think you probably just want to completely remove the logic from `ELFBuilder` for creating a symbol table. With a bit of care, you then should be able to create the symbol table when you need to rather than early, and at the same time remove the whole `EnsureSymtab` logic from the ELFBuilder/ELFReader stack. You'll need to see where things reference the symbol table though to make sure there's no risk of links not being set up properly.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:111
+## and add a brand new one.
+# RUN: llvm-objcopy --allow-broken-links -R .symtab --add-symbol foo=1234 %t %t2
+# RUN: llvm-readelf --sections %t2 | FileCheck %s --check-prefix=SECTIONS
----------------
You shouldn't need `--allow-broken-links`.

`%t` has no symbols in it. I think this test case would be better off using an input file that does contain symbols. I'd recommend splitting this case off into a new file and adding your own YAML with one or more symbols in, showing that the old symbols have been deleted and that new ones have been added. You'll want to show that both the new symbols and the new symtab section header are correct too.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:113
+# RUN: llvm-readelf --sections %t2 | FileCheck %s --check-prefix=SECTIONS
+#
+# SECTIONS: There are 4 section headers
----------------
Nit: The norm is to not put comment markers on blank line.


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

https://reviews.llvm.org/D82935