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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo 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
----------------
rupprecht wrote:
> Can you keep the RUN sections together?
Sure - I tried to group them with the corresponding checks, but I see why it can be good to have all the RUN commands at the top as well.


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:117
+      },
+      /* Internal */ true);
+  if (!AssociatedSections.empty()) {
----------------
rupprecht wrote:
> 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.
Ok, that works - by updating the ToRemove predicate it becomes pretty palatable.

A second non-recursive alternative would to just remove sections and symbols for the associated sections once. I'm not entirely sure if it's valid (or supported by other tools) to have chains of associated sections, but handling that came for free in the recursive form (and by updating ToRemove).


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

https://reviews.llvm.org/D56683