[PATCH] D39769: [llvm-objcopy] Add --strip-all option to remove all non-allocated sections but keep section header table

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Nice work with that investigation.

I think the point I raised at line 177 of llvm-objcopy.cpp suggests that we need a bit more testing of chaining of stripping options. I think you should consider adding a test that performs --remove-section followed by --strip-all (and/or the other way around).



================
Comment at: test/tools/llvm-objcopy/strip-all.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --strip-all %t %t2
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > This test should really have some other sections in it. What sections are actually being stripped here (I assume the symbol table is)? What about debug sections, which are also stripped by GNU objcopy --strip-all?
> Debug sections aren't allocated so they'll be stripped. In this case .symtab, and .strtab are stripped but .shstrtab is not stripped. The maning behind this test was just to check that only the two allocated sections were kept and .shstrtab was kept since the section header table was kept. I could include more allocated and non-allocated sections I suppose but it seems a bit redundant. Now the fact that the .comment section is being kept is news to me, I'm not sure what rule I'm supposed to be following to make that happen.
Expanding the test to cover the different cases like you have done is good - we want to at the least have coverage of each of the different branch cases. However, there is no relocation section, so there probably should be. Ideally, what I'd like is for there to be a section of each different type that can be stripped, along with SHF_ALLOC versions of all those sections, but I could be persuaded that might be over the top.

So, in addition to the cases you've already got, that means a SHT_REL, and a SHT_RELA, and also SHF_ALLOC SHT_REL, SHT_RELA, and SHT_STRTAB (and for completeness a SHF_ALLOC SHT_SYMTAB, although I doubt that this case will ever occur in practice).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:85
+static cl::opt<bool> StripAll("strip-all",
+                              cl::desc("Removes non-allocated sections"));
 static cl::opt<bool> StripSections("strip-sections",
----------------
Given the change in behaviour, I think the help text needs updating. I guess "Removes symbol, relocation, and debug information" should probably be fine.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:177
+      if ((Sec.Flags & SHF_ALLOC) != 0)
+        return false;
+      if (&Sec == Obj->getSectionHeaderStrTab())
----------------
Won't this and the next "return false" override any other "RemovePred" specified earlier? They should probably be "return RemovePred(Sec)" or similar, otherwise you can't do "--strip-all --remove-section=.text" or similar.


Repository:
  rL LLVM

https://reviews.llvm.org/D39769