[PATCH] D55881: [llvm-objcopy] [COFF] Add support for removing symbols

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked an inline comment as done.
mstorsjo added a comment.

In D55881#1338836 <https://reviews.llvm.org/D55881#1338836>, @mstorsjo wrote:

> I realized a few shortcomings in my approach so far (that had gone unnoticed while I've kept my patchset in use for a couple weeks but that just now cropped up) - the fact that I can't use symbol names as unique keys


I'm updating the patch to work around this now, by adding a different unique key to each symbol, to avoid having to use the non-unique name for that. Incidentally I noticed that neither obj2yaml, llvm-readobj or llvm-objdump were capable of differentiating between two symbols with the same name when dumping relocations; I'm adding support for that in D56140 <https://reviews.llvm.org/D56140> for llvm-readobj, and will amend the tests here a bit once that lands, to properly verify that aspect as well.



================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:35
+  Sections.erase(
+      std::remove_if(std::begin(Sections), std::end(Sections),
+                     [ToRemove](const Section &Sec) { return ToRemove(Sec); }),
----------------
jakehehrlich wrote:
> mstorsjo wrote:
> > jakehehrlich wrote:
> > > Can sections reference each other? Can a symbol reference a section? A significant portion of the complexity in removeSection in ELF comes from the fact that we handle those sorts of cases. If you remove a section then depending on circumstance we either remove the referencing object as well or throw an error because you weren't *also* removing the reference.
> > A symbol references a section, and a relocation can reference a symbol. There are cases where sections can reference each other (associative comdat symbols) that I haven't implemented yet - I don't think GNU objcopy does either (it doesn't know about the associative comdat concept at all, afaik). Getting those right would of course be ideal, but they're a bit out of scope of what I'm aiming at initially (basic stripping functionality).
> > 
> > Most of the functionality in this patch also revolves around this - removing symbols that belong to a removed section, etc, but it's rather spread out, and the Object class API isn't easy to use for constructing things from scratch, and updates requires coordinating a few steps.
> Awesome! Good to hear COFF doesn't (at least yet) have that complexity.
Actually, associative comdats are a case where one section (indirectly, via a section definition symbol) references another section. Since this patch only handles symbol removal so far, I'm not touching that quite yet. Once I do, I'll probably first just let the removal do what it does and then error out at the end if there are dangling references - which should save quite a bit of complexity and be just fine for the basic stripping functionality (and shouldn't exclude implementing it differently later if desired).


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

https://reviews.llvm.org/D55881