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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.

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



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/discard-locals.test:29-39
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Size:            64
----------------
You can simplify this a bit, by removing one of the sections, removing the content of the other section, and removing things like the flags, address and alignment fields. You don't need much of an ELF to test symbol discarding.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/discard-locals.test:45-46
+      Section:  .text
+      Value:    0x1000
+      Size:     8
+    - Name:     LocalSection
----------------
The size and value of these symbols is irrelevant, so let's not list them.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/discard-mix-local-and-all.test:52
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
----------------
Same comments as in the other test apply here.


================
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)
----------------
COMP-LOCAL? What does that mean? Probably better to call it something like NO-DISCARD.


================
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;
----------------
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?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:86
   bool DeterministicArchives = true;
-  bool DiscardAll = false;
+  DiscardType DiscardMode = DiscardType::None;
   bool ExtractDWO = false;
----------------
This isn't a boolean option any more, so should be elsewhere in the config.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:331-332
+            StringRef(Sym.Name).startswith(".L"))) &&
+          Sym.Binding == STB_LOCAL && Sym.getShndx() != SHN_UNDEF &&
+          Sym.Type != STT_FILE && Sym.Type != STT_SECTION)
         return true;
----------------
I don't think a number of these cases are being tested, from what I can tell. I'd expect to see the following test cases:
1) normal local symbol (e.g. STT_OBJECT) with .L prefix.
2) other normal local symbol (i.e. without .L prefix).
3) Undefined local symbol with .L prefix.
4) Local file symbol with .L prefix.
5) Local section symbol with .L prefix.
6) Global/weak symbol with .L prefix.
As things stand, the test only tests 1) and 2).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57248