[PATCH] D82274: [llvm-objcopy] Emit error if removing symbol table referenced by SHT_GROUP section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Tip: before updating the diff, select the little "Done" check box on inline comments. This will make it clear to the reviewer that you've read and addressed that comment. The "marked as done" action will then be automatically submitted when you update the diff.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:71
+# SECTIONS-NEXT: NULL
+# SECTIONS-NEXT: .group
+# SECTIONS-NEXT: .text.bar
----------------
Thinking about it, we should show what the value of the .group section's sh_link field here is. It's one of the later columns, so you'll want to expand this line to check up to that point (the rest of the values don't matter, so consider something like: `.group {{.*}} {{.*}} ... 0`. Also update the column header line to extend to the Link column.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:77
+
+## Nothing will change when removing symtab again; tool should exit with return code 0.
+# RUN: llvm-objcopy --allow-broken-links -R .symtab %t %t3
----------------
I think this test case is useful in principle but needs some more work, but the description would probably be better changed to something like:

"Show that llvm-objcopy can handle a group section with a zero sh_link field".


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:78
+## Nothing will change when removing symtab again; tool should exit with return code 0.
+# RUN: llvm-objcopy --allow-broken-links -R .symtab %t %t3
----------------
I assume you didn't mean to just repeat the previous llvm-objcopy run with the exact same input, and that what you actually meant to do was show that the output of the previous run can be used as an input to llvm-objcopy again?

Assuming I've got that right, you need to update your test input/output file. I also don't think "exit with return code 0" is a particularly useful test on its own - `int main(){}` passes that test after all. It would be better to show that the input and output files are identical after the run.


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

https://reviews.llvm.org/D82274