[PATCH] D47052: [llvm-objcopy] Fix the behavior of --strip-* and --keep-symbol

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:226
 
+  bool KeepSymbolsTable = false;
+  if (Obj.SymbolTable) {
----------------
If possible I'd prefer this come after and I'd prefer not using the functions in removeSymbols to keep state. I think it's sufficient to just check that SymbolsToKeep is non-empty for the single explicit section keep.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:228
+  if (Obj.SymbolTable) {
+    Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
+      if ((Config.LocalizeHidden &&
----------------
Not something I want to fix here but I just realized that passing thought the symbol table every time is really inefficient for most use cases. We should somehow change that.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:253
+
+    Obj.removeSymbols([&](const Symbol &Sym) {
+      if (!Config.SymbolsToKeep.empty() &&
----------------
Also same thing here. Could you add TODOs to only remove symbols when there is an option that effects them?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:260
+
+      if ((Config.DiscardAll || Config.StripAll || Config.StripAllGNU ||
+           Config.StripNonAlloc) &&
----------------
I think we can just make Strips imply DiscardAll when a symbol is kept instead of doing this. Alternatively StripAll and StripAllGNU should cause each symbol to be removed and then only remove the symbol table if its empty in those options (which is closer to what GNU objcopy does anyway).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:296
   if (Config.StripAllGNU)
-    RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
+    RemovePred = [RemovePred, &Obj, KeepSymbolsTable](const SectionBase &Sec) {
+      if (KeepSymbolsTable &&
----------------
Instead of duplicating all of these you can add this once at the end as a single keep step

Proposal aggregated from my comments
```
if (!Config.SymbolsToKeep.empty())
  RemovePred = [RemovePred, &Obj](const SectionBase &Sec] {
    if ( /* section is symbol table or symbol table's string table */)
      return false;
    return RemovePred(Sec);
  }
```


Repository:
  rL LLVM

https://reviews.llvm.org/D47052