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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 8 inline comments as done.
mstorsjo added inline comments.


================
Comment at: test/tools/llvm-objcopy/COFF/X86/remove-symbol1.s:11-12
+# RELOCS-NEXT:   Section (1) .text {
+# RELOCS-PRE-NEXT:  0x4 IMAGE_REL_AMD64_REL32 ptr (16)
+# RELOCS-POST-NEXT: 0x4 IMAGE_REL_AMD64_REL32 ptr (15)
+# RELOCS-NEXT:     0xE IMAGE_REL_AMD64_REL32 foo (12)
----------------
jhenderson wrote:
> mstorsjo wrote:
> > jhenderson wrote:
> > > Unless there's a strong need, I'd just drop the (16)/(15) bit at the end, as it allows folding these two lines together, and doesn't really give us anything, since there's only one "ptr" symbol.
> > The point I tried to make in keeping it explicit, is that llvm-objcopy updated the symbol index in this relocation. Granted, if it wouldn't have, the symbol name would have changed as well (or it would have ended up as a broken relocation), but I tried to show it clearly that this reloc actually does refer to a symbol that changed index.
> Okay, that seems reasonable. Might be worth a small comment to show that's what you're doing (i.e. showing that the index has been changed).
Ok, adding a comment here.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:64
+
+  const std::vector<Symbol> &getSymbols() const { return Symbols; }
+  const Symbol *findSymbol(size_t UniqueId) const;
----------------
jhenderson wrote:
> Could this return an `ArrayRef`?
Yup, will change.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:67
+
+  void addSymbols(const std::vector<Symbol> &NewSymbols);
+  void removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
----------------
jhenderson wrote:
> This can take an `ArrayRef`.
Done.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:72
+  // of symbols itself.
+  iterator_range<std::vector<Symbol>::iterator> getMutableSymbols() { return make_range(Symbols.begin(), Symbols.end()); }
+
----------------
jhenderson wrote:
> mstorsjo wrote:
> > Oops, I forgot to clang-format this one - done locally but won't repost a new patch just for that.
> I'd move this method to be alongside `getSymbols`.
Ok, will do.


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

https://reviews.llvm.org/D55881