[PATCH] D62317: [llvm-objcopy] - Strip undefined symbols if they are no longer referenced following --only-section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D62317#1515526 <https://reviews.llvm.org/D62317#1515526>, @grimar wrote:

> > The behaviour you've implemented removes all unreferenced symbols, even if they weren't previously referenced. Is this wise?
>
> This behavior follows GNU, i think it is probably OK. Do you think we should change it?


No, if it follows GNU, keep it as is.

> 
> 
>> Also, have you experimented with other stripping options, e.g. removing a section which references a symbol?
>>  The bug used --only-section, but there might be other similar cases.
> 
> Not yet, but that was my plan (after landing this). I was not going to close the bug before testing
>  other options and fixing them (I expect to see issues, yes).

I think you should do that before landing this, because it might mean the implementation is a little simpler (e.g. we simply always remove unreferenced undefined globals, regardless of the switch).



================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:1
+## Here we want to check that llvm-objcopy removes an undefined symbol
+## if all references to it have been stripped.
----------------
Maybe worth removing "only-section" from the test title, as I think it would make sense for other options to fall under this test too.


================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:10
+# RUN: llvm-readobj --symbols %t2.o | FileCheck %s --check-prefix=BAR
+# BAR: bar
+
----------------
Probably best to put a blank line before the CHECK line, for ease of reading.


================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:14-17
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
----------------
Nit: remove the extra whitespace here and below.


================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:21-22
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000001
+    Content:         BA00000000
----------------
You don't need these fields.


================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:26-29
+    Flags:           [ SHF_INFO_LINK ]
+    Link:            .symtab
+    AddressAlign:    0x0000000000000008
+    EntSize:         0x0000000000000018
----------------
You don't need any of these fields. Ditto below.


================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:48
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    Content:         '90'
----------------
You don't need this.


================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:56
+
+## Check we remove unreferenced undefined symbol, even if it wasn't
+## previously referenced. This follows GNU.
----------------
symbol -> symbols
it wasn't -> they weren't


================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:72
+    Type:          SHT_PROGBITS
+    AddressAlign:  0x0000000000000001
+    Content:       '90'
----------------
You don't need this.


================
Comment at: test/tools/llvm-objcopy/ELF/only-section-strip-undefined.test:75-77
+  - Name:          .keep_me
+    Type:          STT_SECTION
+    Section:       .keep_me
----------------
I don't think you need this symbol.


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:419
 
+    // We want to remove the undefined symbols if the source of reference,
+    // like relocation section was stripped.
----------------
jhenderson wrote:
> remove the undefined -> remove undefined
> the source of reference -like relocation section was stripped > all references have been stripped
Apparently I failed at typing here:

remove the undefined -> remove undefined
the source of reference, like relocation section was stripped -> all references have been stripped


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

https://reviews.llvm.org/D62317