[PATCH] D38335: [llvm-objcopy] Add support for --strip-sections to remove all section headers leaving only program headers and loadable segment data

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 28 01:56:39 PDT 2017


jhenderson added a comment.

I definitely support the new switch, but there are a few issues with this change as it stands:

As mentioned inline, I think quite a few of these changes are not specific to removing all section headers, but rather should be in https://reviews.llvm.org/D38260.

Also, I'm not convinced that removing all sections is the same as removing all section headers, i.e. -R on every section is subtly different from --strip-sections. This is because removing sections one-by-one should still leave the null section header, in my opinion, whereas --strip-sections shouldn't. Essentially there's a slight semantic difference in an ELF without a section header table, and one with no sections.

Finally, there's currently no test for trying to strip the section header string table directly, if there are any remaining sections. I'm not sure it should be an error anyway - it looks like GNU binutils at the very least can handle an ELF with no section header string table, but with section headers, without too much problem, although I'd expect the e_shstrndx field to be set to 0 in this case.



================
Comment at: test/tools/llvm-objcopy/strip-sections.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --strip-sections %t %t2
----------------
This test needs to test the ELF header fields as well.


================
Comment at: tools/llvm-objcopy/Object.cpp:630
 void Object<ELFT>::writeSectionData(FileOutputBuffer &Out) const {
-  for (auto &Section : Sections)
-    Section->writeSection(Out);
+  // First write segment data that needs to be loaded
+  for (auto &Segment : this->Segments) {
----------------
Full stop.

Why do we need the first loop here? What was wrong with the previous behaviour? If the answer is because the section data should be preserved in the PT_LOAD segment regardless of whether a section is stripped or not, then shouldn't this change also be present in D38260?


================
Comment at: tools/llvm-objcopy/Object.cpp:660-661
+  // can remove those as well though and the output should still be sensible.
+  if (ToRemove(*SymbolTable))
+    SymbolTable = nullptr;
+  if (ToRemove(*SectionNames)) {
----------------
This sounds like it's not limited to this change? What's stopping us using -R to strip the .symtab (as long as we also strip any other sections referring to it)?


================
Comment at: tools/llvm-objcopy/Object.cpp:662-670
+  if (ToRemove(*SectionNames)) {
+    // It only makes sense to remove the section header names if we also remove
+    // all sections.
+    if (Iter != Sections.begin())
+      error("Cannot remove " + SectionNames->Name +
+            " because it is the section header string table and some sections"
+            " are being left in the section header table");
----------------
Ditto - should this not be part of D38260?


Repository:
  rL LLVM

https://reviews.llvm.org/D38335





More information about the llvm-commits mailing list