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

Nearly there, just a few small changes to make now, I think.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test:1-2
+## Show that llvm-objcopy can handle a case when we wanted to remove all symbols
+## and add a brand new one.
+
----------------
Perhaps this comment could be simplified slightly:

"Show that llvm-objcopy can replace the symbol table with a new one."


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test:4
+
+# RUN: yaml2obj %s -o %t
+
----------------
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>
```


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test:36
+
+# RUN: llvm-readelf -s %t | FileCheck --check-prefix=SYMBOLS %s
+
----------------
I don't think you need this llvm-readelf line, or the corresponding checks, since it is just verifying yaml2obj does the right thing, but we can assume it does, since there is testing for that in the yaml2obj testing.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test:49
+
+# SECTIONS: There are 5 section headers
+# SECTIONS:      [Nr] Name      Type   Address Off    Size   ES     Flg Lk Inf
----------------
I assume you are using it to show the sh_link value of the new symbol table is correct? If so, you a) can add `-S` to the llvm-readelf call above and b) just use the same check-prefix for everything (`CHECK` is fine, since you will only have the one FileCheck command).


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-new-symtab.test:57
+
+# RUN: llvm-readelf --sections %t2 | FileCheck %s --check-prefix=SECTIONS
+
----------------
Aside from the fact that the checks and commands are not in a sensible order, you don't need two separate commands, as I note in my previous comment.


================
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;
----------------
I think you want something, probably an `assert`, that prevents this function being called if there already is a symbol table in the `Object`.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1886
+  StringTableSection *StrTab = nullptr;
+  for (auto &Sec : sections()) {
+    if (Sec.Type == ELF::SHT_STRTAB && !(Sec.Flags & SHF_ALLOC)) {
----------------
Whilst you're moving this code, please change this to not use `auto`. I was a bit lax earlier in my reviewing of this code when it was all first written, and I find `auto` in this context makes it harder to figure out exactly what sort of section class `Sec` actually is.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1042
 
+  void createSymbolTable();
+
----------------
I'd call the new function `addSymbolTable` (or possibly `addNewSymbolTable` and put it next to the `addSection` function in the header and code as conceptually, the two are related.


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

https://reviews.llvm.org/D82935