[PATCH] D57248: [llvm-objcopy] Support -X|--discard-locals.

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked 9 inline comments as done.
rupprecht added a comment.

In D57248#1371694 <https://reviews.llvm.org/D57248#1371694>, @mstorsjo wrote:

> > I am not sure about COFF local symbols: those appear to also use .L in most cases, but also use just L in other cases, so for now I am just leaving it unimplemented there.
>
> AFAIK they only use `.L` - however GNU objcopy seems to strip out all local symbols that start with either `.L` or `L`. On arches other than i386, where symbols have no prefix otherwise, this actually seems to get rid of any normal local symbol that happens to start with such a char.
>
> But leaving it unimplemented here is fine by me.


I would prefer to leave it unimplemented as part of this patch, because I don't know anything about COFF :) but I can follow up with an attempt to add it specifically for COFF once all this wiring is in place

In D57248#1373312 <https://reviews.llvm.org/D57248#1373312>, @jhenderson wrote:

> Please add a test for the case where the symbol that would be discarded is referenced by a relocation, and show that it matches GNU behaviour, or at least that we are consistent with our existing behaviour.


Added a test -- we're consistent with existing behavior, but not with GNU here. i.e.:

  $ llvm-objcopy --discard-all <yaml obj from discard-locals-rel.test> /tmp/foo.o
  llvm-objcopy: error: not stripping symbol '.L.rel' because it is named in a relocation.
  $ llvm-objcopy --discard-locals <yaml obj from discard-locals-rel.test> /tmp/foo.o
  llvm-objcopy: error: not stripping symbol '.L.rel' because it is named in a relocation.
  $ objcopy --discard-locals <yaml obj from discard-locals-rel.test> /tmp/foo.o
  # No errors, copies the object and avoids stripping .L.rel

I'm not sure if this is a problem in practice; I can test a bit more once this patch is committed. If it is a problem, it's an existing problem with --discard-all.

> Another behaviour to check is what happens with SHN_ABS and SHN_COMMON .L* local symbols?

Added these tests, and verified we're consistent with GNU objcopy --discard-locals for these test files.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/discard-mix-local-and-all.test:100
+# LOCAL-NEXT:        }
+# COMP-LOCAL-NEXT:   Symbol {
+# COMP-LOCAL-NEXT:     Name: .L.str (1)
----------------
jhenderson wrote:
> COMP-LOCAL? What does that mean? Probably better to call it something like NO-DISCARD.
COMP-LOCAL is for "Compiler-generated local symbols" as it is called in documentation (at least it is in gnu man pages & as I added to --help in this patch); clarified with a comment at the top and expanded to COMPILER-LOCAL


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:351-355
+  if (InputArgs.hasArg(OBJCOPY_discard_all, OBJCOPY_discard_locals))
+    Config.DiscardMode =
+        InputArgs.hasFlag(OBJCOPY_discard_all, OBJCOPY_discard_locals)
+            ? DiscardType::All
+            : DiscardType::Locals;
----------------
jhenderson wrote:
> I just want to confirm that this matches GNU objcopy. If using both --discard-all and --discard-locals in GNU objcopy, does the last triumph, or does --discard-all always triumph, regardless of order?
Yes, see here: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=binutils/objcopy.c;h=0e17b863d5a7d4b95b1dfbc7aa8d6b683b85d592;hb=HEAD#l4453

```
  while ((c = getopt(...)) {
      switch (c) {
...
	case 'x':
	  discard_locals = LOCALS_ALL;
	  break;
	case 'X':
	  discard_locals = LOCALS_START_L;
	  break;
```

i.e. last one wins. This can be double checked with `objcopy -x -X` vs `objcopy -X -x`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57248