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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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)
----------------
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).


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:20-23
+  for (Symbol S : NewSymbols) {
+    S.UniqueId = NextSymbolUniqueId++;
+    Symbols.emplace_back(S);
+  }
----------------
Not sure it's actually any simpler, but you could use std::transform, to do this. Approximate untested code:
```
std::transform(NewSymbols.begin(),
  NewSymbols.end(),
  std::back_inserter(Symbols),
  [&NextSymbolUniqueID](Symbol&S){ S.UniqueId = NextSymbolUniqueId++;});
```
Okay, now that I type it out, this is probably less readable too, so feel free to completely ignore it!


================
Comment at: tools/llvm-objcopy/COFF/Object.h:27-28
+struct Relocation {
+  Relocation() {}
+  Relocation(const object::coff_relocation &R) : Reloc(R) {}
+
----------------
mstorsjo wrote:
> jhenderson wrote:
> > Are these constructors really necessary? Won't they be generated by default?
> Sorry, I forgot to reply on this one yesterday. They're indeed unnecessary if using the right syntax for initializing, I didn't think of the new C++11 syntax for doing that without explicit constructors.
I'm happy that you changed this, but I now realise that I misread the original, and probably wouldn't have bothered commenting if I'd realised it was a conversion constructor, not a generic copy constructor!


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


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


================
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()); }
+
----------------
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`.


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

https://reviews.llvm.org/D55881