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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/remove-section.test:22
+
+# CHECK: SectionHeaderCount: 6
+
----------------
As things stand, this test only shows that .test2 is not between .test1 and .test3. I think there are two ways to change this: 1) CHECK-NOT: .test2 between every block of CHECKs or 2) CHECK for each section (not just .test1 and .test3) by name specifically.


================
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) +
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Why has this changed?
> Oh whoops. I forgot to mention this. It was wrong before. I was using StringTableSection for what under the hood was actually a Section. This was fine because I wasn't using anything that wasn't in SectionBase here. Additionally dynamic string tables have SHT_STRTAB so the cast went though just fine. I think to avoid this mistake in the future I should probably add a check that StringTableSection is not allocated.
Ok, that sort of makes sense. In that case, however, could you make this change in a separate review/commit, please, as it's not really related to removing sections.


================
Comment at: tools/llvm-objcopy/Object.h:199
 
-template <class SymTabType> class RelocationSectionBase : public SectionBase {
+class DefinedOnRelocSection : public SectionBase {
+protected:
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > What is the purpose of this new class? Can't we just use RelocationSectionBase?
> It lets me cast both to DefinedOnRelocSection so that I can handle both cases uniformly. 
Ah, okay, I forgot that we had templated RelocationSectionBase. In that case, could we rename them, because I do not understand what "DefinedOnRelocSection" is supposed to represent - what is defined here?

I would seriously consider renaming DefinedOnRelocSection "RelocationSectionBase", since that is the base class for relocation sections, and then maybe RelocationSectionBase to "RelocSectionWithSymtabBase" or similar.


================
Comment at: tools/llvm-objcopy/Object.h:216
   SymTabType *Symbols = nullptr;
   SectionBase *SecToApplyRel = nullptr;
 
----------------
I think this member should now be deleted?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:59
                                "\n\tbinary"));
+cl::opt<std::string> ToRemove("R", cl::desc("Remove a specific section"));
 
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > objcopy also allows --remove-section as an alias. It also allows specifying the option multiple times to strip multiple sections.
> I'll add --remove-section as an alias. Would you take  making ToRemove a cl::list as a todo?
Yes, that's fine with me.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:72
+    Obj->removeSections(
+        [&](const SectionBase &Sec) -> bool { return ToRemove == Sec.Name; });
+  }
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Why a lambda here? Also, the explicit return type is unnecessary.
> What alternative are you proposing? Or are you just asking why this method takes a lambda? That's because many other conditions for removal will be added. This interface should cover all of them however.
The alternative would be to pass in the section name(s) to the function and test them there, but if the plan is to add other ways to remove sections, then this is fine with me. The explicit return type is still unnecessary, I believe.


Repository:
  rL LLVM

https://reviews.llvm.org/D38260