[PATCH] D46341: [llvm-objcopy] Add --discard-all (-x) option

[Paul Semel via Phabricator via llvm-commits]

paulsemel added inline comments.


================
Comment at: tools/llvm-objcopy/Opts.td:76
+def discard_all : Flag<["-", "--"], "discard-all">,
+                      HelpText<"Remove all non-global symbols">;
+def x : Flag<["-"], "x">,
----------------
jhenderson wrote:
> I know this is what gnu objcopy says, but non-global means not STB_GLOBAL to me, but it doesn't affect some other symbol types either (i.e. STB_WEAK, STT_FILE, STT_SECTION). Could you change the help text to match what it actually does, please.
Yes, I know, this is a really bad help text. But I was out of ideas, as the real behavior of this option is really tricky... Any thoughts about it guys ?
I was thinking of  "Remove all but file and section local symbols" , but even for me that sounds really bad to my ears, so..


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:344
+    Obj.SymbolTable->removeSymbols([&](Symbol &Sym) {
+      if (Config.DiscardAll && Sym.Binding == STB_LOCAL &&
+          Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
----------------
alexshap wrote:
>   return Config.DiscardAll && Sym.Binding == STB_LOCAL &&
>              Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
>              Sym.Type != STT_SECTION
Actually, as there is other options that will use this `removeSymbols` function, this choice was made to make it clearer. I was thinking about something like :

```
if (Config.DiscardAll && Blabla...)
    return true;

if (Config.DiscardLocals && Blabla..)
    return true;

return false;
```

I personally find it easier to read, even if this is not the most optimized way for doing it. What do you think ? 


Repository:
  rL LLVM

https://reviews.llvm.org/D46341