[PATCH] D46567: [llvm-strip] Add support for -remove-section

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

The overall approach looks fine to me. I think I agree with your comments about not overloading remove-section.test. I think your solution is actually better in this case. We don't have much combination testing of switches in llvm-objcopy, if I remember correctly, so having distinct llvm-strip tests where we are effectively testing some new combo is reasonable, in my opinion, but as noted by @jakehehrlich, we should definitely check the equivalent llvm-objcopy equivalent in the same test. Essentially if we can do some invocation of llvm-objcopy and some (potentially different) invocation of llvm-strip and get the same output in both cases, there should be a test containing them both. If the behaviour cannot be replicated by llvm-strip, then we should only test llvm-objcopy. That being said, I'm not opposed to a --no-strip switch, although I'm not as certain of the value.

LGTM, with lots of small comments, though I'm not too bothered by any of them, so if you disagree with any, feel free to go ahead and commit anyway.



================
Comment at: test/tools/llvm-objcopy/remove-section.test:1
 # RUN: yaml2obj %s > %t
+# RUN: cp %t %t1
----------------
No issues with the changes made in this test, but since this isn't testing llvm-strip, I'd prefer it if they were in a separate commit. Feel free to commit it without further review.


================
Comment at: test/tools/llvm-objcopy/remove-section.test:6
 
+# RUN: cmp %t %t1
+# RUN: llvm-objcopy -remove-section=.test2 %t1 %t3
----------------
Probably worth commenting why we do a check here. There are probably several other cases in other tests where we do something similar too, which should probably be updated.


================
Comment at: test/tools/llvm-objcopy/strip-and-remove.test:1
+# RUN: yaml2obj %s > %t
+# RUN: cp %t %t1
----------------
What are your thoughts on renaming this test to strip-all-and-remove.test? That way it's clear that we are testing the combination of the two switches of llvm-objcopy, one of which is effectively always on for llvm-strip. It also allows for easier distinguishing from strip-debug.


================
Comment at: test/tools/llvm-objcopy/strip-and-remove.test:17
+Sections:
+  - Name:            .debugfoo
+    Type:            SHT_PROGBITS
----------------
If this is intended to be a real debug section, it probably is worth naming it in the same style as other debug sections i.e. .debug_foo.


================
Comment at: test/tools/llvm-objcopy/strip-and-remove.test:44
+# CHECK-NOT: Name: .text.bar
+# CHECK-NOT: Name: .debugfoo
+
----------------
Should we have a CHECK-NOT: Name: .symtab here?

Strictly speaking, an empty symbol table is distinct from a missing one.


================
Comment at: test/tools/llvm-objcopy/strip-debug-and-remove.test:17
+Sections:
+  - Name:            .debugfoo
+    Type:            SHT_PROGBITS
----------------
See my comments in the other test.


Repository:
  rL LLVM

https://reviews.llvm.org/D46567