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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:136
+  // Actually do removals of symbols.
+  Obj.removeSymbols([&](const Symbol &Sym) {
+    if (!Config.SymbolsToKeep.empty() &&
----------------
The two styles by which removeSymbols and removeSections were written was a historical accident in ELF. We should pick one and stick to it here. I was going to say I don't really care which but I'll retract that statement say that using the style that was used in `removeSymbols` is to be preferred. I think it will both perform better and be less confusing to incoming devs.


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:144
+
+    if (!Config.SymbolsToRemove.empty() &&
+        is_contained(Config.SymbolsToRemove, Sym.Name)) {
----------------
I think supporters of checking `empty()` prior to `is_contianed` changed their minds. This should be removed in all checks.


================
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); }),
----------------
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.


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:40
+
+void Object::initRawSymbolTable() {
+  for (Symbol &S : Symbols) {
----------------
This seems like it is perhaps something the Writer should do? I'm not clear on the intention here.


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:48
+
+void Object::setReferencedSymbols(StringRef Filename) {
+  // This function is meant to be used before pruning any symbols.
----------------
Again don't follow the error handling that I used in ELF code. You don't need to pass the otherwise useless Filename if you return an error because you can add that information when you report the error in whatever handles this. Also sometimes it won't even be a file (see -I binary for instance)


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:58
+      if (Reloc.SymbolTableIndex >= RawSymbolTable.size())
+        reportError(Filename,
+                    make_error<StringError>("SymbolTableIndex out of range",
----------------
I know I reportError or call error all of the place, but it's best to avoid that and instead propogate the Error back. I forgot to check for that in the previous patch. Doing this makes writing tools easy but makes converting this into a library hard.


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:70
+
+void Object::initRelocTargets(StringRef Filename) {
+  // This function must be called before pruning any symbols.
----------------
Ditto on filename and reporting and comment about RawSymbolTable not belonging in general.


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:100
+    assert(Sec.Relocs.size() == Sec.RelocTargets.size());
+    for (size_t I = 0, E = Sec.Relocs.size(); I < E; I++) {
+      auto It = SymMap.find(Sec.RelocTargets[I]);
----------------
Why can't this be a loop over Sec.Relocs? The only blocker seems to be the use of `Sec.RelocTargets[I]` but it seems that's just a parallel array which is force you to write it like this.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:31
+
+  // Fields used by some transformations in objcopy, but not necessary
+  // for actually writing back to a file.
----------------
Depending on what you mean here, this makes me think this should not be necessary. Can you elaborate?


================
Comment at: tools/llvm-objcopy/COFF/Object.h:33
+  // for actually writing back to a file.
+  std::vector<StringRef> RelocTargets;
+  size_t Idx;
----------------
Aside from weather these should actually exist, (Idx probably should from my experience but I'm skeptical about RelocTargets) aren't you maintaining a parallel array array here? Why not just have a proper Reloc struct and put both the coff_relocation and the Target in it? That would improve some code I'm going to complain about below.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:44
+  // for actually writing back to a file.
+  bool Referenced = false;
 };
----------------
Yeah this was the sort of thing we eventually compromised on but we agreed that it was a result of poor structure in the ELF code. Ideally this sort of thing should not be in Symbol; we just didn't find a way around it that didn't require significant upheaval in the code. Since we're in an earlier stage with COFF code we can find a better way that storing this with the Symbol itself perhaps?


================
Comment at: tools/llvm-objcopy/COFF/Object.h:65
+  // for actually writing back to a file.
+  std::vector<Symbol *> RawSymbolTable;
+
----------------
Seems like a field that only the writer should ever know about.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:71
+  // StringRef taken as parameter for error reporting.
+  void initRawSymbolTable();
+  void setReferencedSymbols(StringRef Filename);
----------------
Can you elaborate on what the intended semantics of this is? I'd kind of prefer the interface of Object be that you can construct a blank object, build it up step by step having a valid object along the way as you go, and then write it when possible. The exception to this is when parts of an Object reference each other you need to allow for temporary invalidity of some form or another while the cycle is being closed.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:72
+  void initRawSymbolTable();
+  void setReferencedSymbols(StringRef Filename);
+
----------------
If possible its most ideal to keep this true as you go rather than setting it all at once. We found that wasn't easily possible to do correctly in the ELF code 1) That might not be true here but even if it is 2) A more generic interface to this would be to use updateSymbols which is the sort of generic operation on an Object that I'd like to see used in favor of something specific like this.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:74
+
+  void initRelocTargets(StringRef Filename);
+  void updateRelocTargets(StringRef Filename);
----------------
ditto.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:75
+  void initRelocTargets(StringRef Filename);
+  void updateRelocTargets(StringRef Filename);
 };
----------------
Can you describe what this does a bit more?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55881