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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:260
+
+      if ((Config.DiscardAll || Config.StripAll || Config.StripAllGNU ||
+           Config.StripNonAlloc) &&
----------------
jakehehrlich wrote:
> 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).
  >StripAll and StripAllGNU should cause each symbol to be removed - 
- it turns out that if at least one symbol specified by -K is kept, than all the local symbols are kept as well, that's what both objcopy and strip do, originally i didn't notice that and the code was much simpler. Anyway - i'll look into this tomorrow,  kinda tired and want to sleep, maybe i sent this diff too quickly, want to think a bit more.


================
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 &&
----------------
alexshap wrote:
> jakehehrlich wrote:
> > 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);
> >   }
> > ```
> that's what i originally did,
> it doesn't work correctly, or, at least not what binutils objcopy does.
i will look again tomorrow


Repository:
  rL LLVM

https://reviews.llvm.org/D47052