[PATCH] D39021: [llvm-objcopy] Add support for --only-keep/-j and --keep

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Could we have test cases for the following please:

1. --only-keep + some way of stripping each of .symtab, .strtab, and .shstrtab.
2. Show that --keep overrides --only-keep (i.e. sections not specified in --only-keep will still be kept if specified by --keep).

Two other tests mentioned inline, that are --only-keep equivalents to --keep.

As an aside, 2) might be a slightly surprising behaviour to people due to the name of the switches, but I don't think that's a big issue.

Also lots of small nits in the comments.



================
Comment at: test/tools/llvm-objcopy/basic-keep.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -strip-non-alloc -keep=.test %t %t2
----------------
Could we have an -only-keep version of this test, please. This will demonstrate that --only-keep=.test has priority over e.g. --strip-non-alloc.


================
Comment at: test/tools/llvm-objcopy/explicit-keep-remove.test:2
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -R=.test -keep=.test %t %t2
+# RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
----------------
Could you add an -only-keep version of this test as well, please.


================
Comment at: test/tools/llvm-objcopy/keep-multi.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -strip-non-alloc -keep=.test -keep=.test3 %t %t2
----------------
Could you rename this test, please, to "keep-many.test" for consistency with "only-keep-many.test" (or vice versa, I don't mind which).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:156
+// This function handles the high level operations of GNU objcopy including
+// handeling command line options. It's important to outline certain properties
+// we expect to hold of the command line operations. Any operation that "keeps"
----------------
handeling -> handling


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:159
+// should keep regardless of a remove. Additionally any removal should respect
+// any previous removals. Lastly weather or not something is removed shouldn't
+// depend a) on the order the options occur in or b) on some opaque priority
----------------
weather -> whether


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:241
+    RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
+      // Explicitlly keep these sections regardless of previous removes.
+      if (std::find(std::begin(OnlyKeep), std::end(OnlyKeep), Sec.Name) !=
----------------
Explicitlly -> Explicitly


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:245
+        return false;
+      // Allow all implicit removes
+      if (RemovePred(Sec)) {
----------------
jhenderson wrote:
> Please add a new line before and after this if clause (with its comment), to clearly indicate what the comment refers to.
Full stop.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:245-248
+      // Allow all implicit removes
+      if (RemovePred(Sec)) {
+        return true;
+      }
----------------
Please add a new line before and after this if clause (with its comment), to clearly indicate what the comment refers to.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:249-254
+      if (Obj->getSectionHeaderStrTab() == &Sec) {
+        return false;
+      }
+      if (Obj->getSymTab() == &Sec || Obj->getSymTab()->getStrTab() == &Sec) {
+        return false;
+      }
----------------
I think it would be nice to have a comment around these two ifs to say something like "keep special sections."


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:255
+      }
+      // Remove everything else (but keep some special sections)
+      return true;
----------------
Full stop.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:262
+    RemovePred = [RemovePred](const SectionBase &Sec) {
+      // Explicitlly keep these sections regardless of previous removes
+      if (std::find(std::begin(Keep), std::end(Keep), Sec.Name) !=
----------------
Explicitlly -> Explicitly. Also full stop.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:266
+        return false;
+      // Otherwise defer to RemovePred
+      return RemovePred(Sec);
----------------
Full stop.


Repository:
  rL LLVM

https://reviews.llvm.org/D39021