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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: test/tools/llvm-objcopy/COFF/remove-section.yaml:29
+#
+# RUN: llvm-objcopy -R .bss %t.in.o %t.remove-bss.o
+# RUN: llvm-objdump -section-headers %t.remove-bss.o | FileCheck %s --check-prefix=SECTIONS-REMOVE-BSS
----------------
Can you keep the RUN sections together?


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:32
+  // Perform the actual section removals.
+  Obj.removeSections([&](const Section &Sec) {
+    if (is_contained(Config.ToRemove, Sec.Name))
----------------
Use explicit capture of &Config


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:99
+  Sections.erase(std::remove_if(std::begin(Sections), std::end(Sections),
+                                [&](const Section &Sec) {
+                                  bool Remove = ToRemove(Sec);
----------------
Use explicit capture of &ToRemove, &RemovedSections


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:117
+      },
+      /* Internal */ true);
+  if (!AssociatedSections.empty()) {
----------------
I think it might be clearer to use an iterative approach, and possibly be able to remove the `Internal` var, e.g. logically something like this:

```
do {
  Sections.erase(...);
  Symbols.erase(...);
  ToRemove = [](...){ return AssociatedSections.count(Sec.UniqueId) == 1; }
} while (!AssociatedSections.empty());
updateSections();
updateSymbols();
```

I dunno though. It might be a little awkward, but recursive stuff just seems a little harder to debug, so it might be worth it.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:81
+  void removeSymbols(function_ref<bool(const Symbol &)> ToRemove,
+                     bool Internal = false);
 
----------------
`Internal` is not very clear -- i.e. it's clear that it's for internal use, but not really clear as to how it affects the call. (Ditto for removeSections).


================
Comment at: tools/llvm-objcopy/COFF/Object.h:87
 
+
+  ArrayRef<Section> getSections() const { return Sections; }
----------------
nit: extra blank line


================
Comment at: tools/llvm-objcopy/COFF/Object.h:111
+  ssize_t NextSectionUniqueId = 1; // Allow a UniqueId 0 to mean undefined.
+
+
----------------
nit: extra blank line


================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:116
+      Sym.TargetSectionId = SymRef.getSectionNumber();
+    else if (static_cast<uint32_t>(SymRef.getSectionNumber() - 1) >=
+             Sections.size())
----------------
Consider having the error flow as the final condition instead of in the middle


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56683/new/

https://reviews.llvm.org/D56683