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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Something that's worth noting is that section symbols often don't actually have a name. Some tools use their section name instead for clarity. I'm wondering if this might cause an issue for such a section symbol for a section named ".L..." e.g. ".LLVM.Custom.Section" or whatever? Definitely it deserves including in the testcase.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/discard-locals.test:37-39
+    - Name:     LocalSection
+      Type:     STT_SECTION
+      Section:  .text
----------------
You can probably get rid of this and the LocalFile symbol below, and just keep the .L versions of those symbols. They don't provide any additional coverage.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/discard-locals.test:54-56
+    - Name:     Global
+      Type:     STT_FUNC
+      Section:  .text
----------------
Ditto.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/discard-locals.test:60-64
+    - Name:     .L.undefined
+    - Name:     .L.abs
+      Index:    SHN_ABS
+    - Name:     .L.common
+      Index:    SHN_COMMON
----------------
These aren't local, so won't be discarded anyway (and therefore aren't testing anything). They should probably be in the local list...


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

https://reviews.llvm.org/D57248