[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 inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test:44
+# CHECK-NEXT: [ 3] .shstrtab
+# CHECK-NEXT: [ 4] .symtab   {{.+}} {{.+}} {{.+}} {{.+}} {{.+}}      3  {{.+}}
+
----------------
Since this is a new section, I'd check the values are all correct, rather than skipping them. Don't forget the alignment column!


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test:4
+
+# RUN: yaml2obj %s -o %t
+
----------------
jhenderson wrote:
> In general, I find tests easier to follow when related `RUN` lines are kept together. Thus I'd go:
> 
> ```
> # RUN: yaml2obj ...
> # RUN: llvm-objcopy ...
> # RUN: llvm-readelf ... | FileCheck %s
> 
> # CHECK: ...
> 
> <YAML>
> ```
You haven't moved the YAML to the end, as I asked...


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1884
+void Object::createSymbolTable() {
+  // Reuse an existing SHT_STRTAB section if it exists.
+  StringTableSection *StrTab = nullptr;
----------------
jhenderson wrote:
> I think you want something, probably an `assert`, that prevents this function being called if there already is a symbol table in the `Object`.
Add a `&& "some explanatory message` to this assert.


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

https://reviews.llvm.org/D82935