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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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
----------------
jhenderson wrote:
> 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.
To assist with debugging, I'd use a different output file, so that the input file isn't overwritten. I'd them `cmp` the output with the input, since they should match. As things stand, this is still no better than an empty main, since you are only checking that llvm-objcopy returns 0, and not that it actually does what you expect too.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:68-69
+
+# SECTIONS: There are 6 section headers
+# SECTIONS: Name
+# SECTIONS-NEXT: NULL
----------------
I'd add extra spaces to make this line up with the rows below. Additionally, I'd add the headers for the columns you have to match below, to clarify which you are actually checking.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:71
+# SECTIONS-NEXT: NULL
+# SECTIONS-NEXT: .group {{.*}} {{.*}} {{.*}} {{.*}} 0 0 {{.*}}
+# SECTIONS-NEXT: .text.bar
----------------
I made a slight mistake in my original proposal - these should be `{{.+}}`. Additionally, you don't need to check anything after the fields you care about.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/group.test:77
+
+## Show that llvm-objcopy can handle a group section with a zero sh_link field.
+# RUN: llvm-objcopy --allow-broken-links -R .symtab %t3
----------------
On further look, this should be "zero sh_link and sh_info fields" instead of just "a zero sh_link field" I think?


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

https://reviews.llvm.org/D82274