[PATCH] D38260: [llvm-objcopy] Add support for removing sections

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

There are no tests here for what happens to segments. Two cases I can think of: sections between segments, sections in segments.



================
Comment at: test/tools/llvm-objcopy/reloc-error-remove-symtab.test:8
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
----------------
Minor quibble, but this should probably be ET_REL, since it's a relocatable object file.


================
Comment at: test/tools/llvm-objcopy/remove-section.test:3
+# RUN: llvm-objcopy -R=.test2 %t %t2
+# RUN: llvm-readobj -sections %t2 | FileCheck %s
+
----------------
Could you please check the ELF header here, as well, and show that e_shnum is correct.


================
Comment at: tools/llvm-objcopy/Object.cpp:157
+    error("String table " + SymbolNames->Name +
+          " cannot be removed because it is referenced by the symbol table");
+  }
----------------
"referenced by the symbol table" -> "referenced by a symbol table", or "referenced by the symbol table" + Name


================
Comment at: tools/llvm-objcopy/Object.cpp:299
 void SectionWithStrTab::initialize(SectionTableRef SecTable) {
-  setStrTab(SecTable.getSectionOfType<StringTableSection>(
-      Link,
-      "Link field value " + Twine(Link) + " in section " + Name + " is invalid",
-      "Link field value " + Twine(Link) + " in section " + Name +
-          " is not a string table"));
+  setStrTab(SecTable.getSection(Link,
+                                "Link field value " + Twine(Link) +
----------------
Why has this changed?


================
Comment at: tools/llvm-objcopy/Object.cpp:635
+    std::function<bool(const SectionBase &)> ToRemove) {
+
+  auto Iter =
----------------
This function strikes me as overly complex for what is doing. I think the SecsToRemove set and the second stable_partition can be removed by changing the first one's predicate to test both the section itself for removal, and if it is a relocation section, the section it refers to:
```
if (ToRemove(*Sec))
  return false;
if (auto RelSec = dyn_cast<DefinedOnRelocSection>(Sec.get()))
  return !ToRemove(*RelSec->SecToApplyRel);
return true;
```


================
Comment at: tools/llvm-objcopy/Object.cpp:652
+  // Now make sure there are no remaining references to the sections that will
+  // be removed. Sometimes it is impossible to remove a reference so we throw
+  // errors here.
----------------
"... throw errors here." -> "... emit an error here instead."


================
Comment at: tools/llvm-objcopy/Object.h:199
 
-template <class SymTabType> class RelocationSectionBase : public SectionBase {
+class DefinedOnRelocSection : public SectionBase {
+protected:
----------------
What is the purpose of this new class? Can't we just use RelocationSectionBase?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:59
                                "\n\tbinary"));
+cl::opt<std::string> ToRemove("R", cl::desc("Remove a specific section"));
 
----------------
objcopy also allows --remove-section as an alias. It also allows specifying the option multiple times to strip multiple sections.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:72
+    Obj->removeSections(
+        [&](const SectionBase &Sec) -> bool { return ToRemove == Sec.Name; });
+  }
----------------
Why a lambda here? Also, the explicit return type is unnecessary.


Repository:
  rL LLVM

https://reviews.llvm.org/D38260