[PATCH] D46369: [llvm-objcopy] Add --strip-symbol (-N) option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Side thought: we should probably ensure all our messages reporting sections include the section indexes, since section names don't have to be unique (although it's unusual for them to be otherwise).



================
Comment at: test/tools/llvm-objcopy/strip-symbol-group.test:1
+# RUN: yaml2obj %s > %t
+# RUN: not llvm-objcopy -N foo %t %t2 2>&1 | FileCheck %s
----------------
I'd rename this test "strip-group-symbol.test", as the current name is a little confusing.


================
Comment at: test/tools/llvm-objcopy/strip-symbol-reloc.test:1
+# RUN: yaml2obj %s > %t
+# RUN: not llvm-objcopy -N Local %t %t2 2>&1 | FileCheck %s
----------------
Similarly, please rename this test to "strip-reloc-symbol.test" or similar.


================
Comment at: test/tools/llvm-objcopy/strip-symbol-reloc.test:26
+  Local:
+    - Name:     Local
+      Type:     STT_FUNC
----------------
Could you give these symbols different names, please, that don't indicate something to do with symbol bindings, e.g. 'foo' and 'bar'

We also probably need only 1 symbol.


================
Comment at: test/tools/llvm-objcopy/strip-symbol.test:20
+  Local:
+    - Name:     Local
+      Type:     STT_FUNC
----------------
As noted above, the bindings are irrelevant, so please rename these symbols to generic names.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:349
 
-    Obj.SymbolTable->removeSymbols([&](const Symbol &Sym) {
+    Obj.removeSymbols([&](const Symbol &Sym) {
       if (Config.DiscardAll && Sym.Binding == STB_LOCAL &&
----------------
paulsemel wrote:
> jakehehrlich wrote:
> > Just thought about this. Can we do symbol removals before symbol updates?
> Well, I took a look and, to me, it doesn't seem that there is technical reasons not to do it.
> For the moment, I don't think this is really relevant, as the user might explicitly enter collapsing options to trigger the counter performance (like LocalizeHidden and DiscardAll for example). Anyway, for the future options I'm planning to implement, I'm pretty sure that the collapsing will be less evident to the user (and in all cases, we might not count on the user not to enter collapsing options, so we might do it anyway).
> If you want, I can propose this change in its own patch, to get better track of changes we make ? :)
I agree with Paul that this should be its own patch. I was thinking about the comment myself, but there are a number of cases we need to consider, as he's suggested, where it's unclear which we should do first (and I'd suggest it follow GNU objcopy if sensible).


Repository:
  rL LLVM

https://reviews.llvm.org/D46369