[PATCH] D61672: [llvm-objcopy] Allow strip symtab in executables and DSOs

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 marked 3 inline comments as done.
evgeny777 added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded2.test:20
+    Size:            64
+DynamicSymbols:
+  - Name:     foo
----------------
jhenderson wrote:
> What's the point of having DynamicSymbols here?
The point is to test that dynamic symbols are not touched by --strip-unneeded run on executable. Do you think it's not needed?


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded2.test:29-32
+    Type:     STT_FUNC
+    Size:     8
+    Section:  .text
+    Value:    0x1008
----------------
jhenderson wrote:
> I think you can simplify this test by removing most of these fields. The only relevant fields are the Binding, Section and Name fields, I believe. Also, what happens for undefined STB_GLOBALs in GNU objcopy? There should be a test for that case here.
> Also, what happens for undefined STB_GLOBALs in GNU objcopy?

Removal. .symtab is not used by dynamic loader, so it's not needed. Undefined STB_GLOBAL symbols are placed in .dynsym by static linker for runtime resolution.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1590-1592
+Error Object::removeUnneededSections() {
+  // We can remove trivial symbol table from non-relocatable objects
+  if (isRelocatable() || SymbolTable == nullptr || !SymbolTable->isTrivial())
----------------
rupprecht wrote:
> I don't think this part needs to be guarded by `isRelocatable()`. Even GNU objcopy will drop the symbol table if it's empty (only contains the null symbol).
> 
> Demo:
> ```
> $ cat /tmp/yaml.txt
> --- !ELF
> FileHeader:
>   Class:           ELFCLASS64
>   Data:            ELFDATA2LSB
>   Type:            ET_REL
>   Machine:         EM_X86_64
> Sections:
>   - Name:          .text
>     Type:          SHT_PROGBITS
> Symbols:
> $ yaml2obj /tmp/yaml.txt > /tmp/null.o
> $ objcopy /tmp/null.o /tmp/null-copy.o
> $ readelf -SW /tmp/null.o /tmp/null-copy.o
> 
> File: /tmp/null.o
> There are 5 section headers, starting at offset 0x40:
> 
> Section Headers:
>   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
>   [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
>   [ 1] .text             PROGBITS        0000000000000000 000180 000000 00      0   0  0
>   [ 2] .symtab           SYMTAB          0000000000000000 000180 000018 18      3   1  8
>   [ 3] .strtab           STRTAB          0000000000000000 000198 000001 00      0   0  1
>   [ 4] .shstrtab         STRTAB          0000000000000000 000199 000021 00      0   0  1
> ...
> File: /tmp/null-copy.o
> There are 3 section headers, starting at offset 0x58:
> 
> Section Headers:
>   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
>   [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
>   [ 1] .text             PROGBITS        0000000000000000 000040 000000 00      0   0  1
>   [ 2] .shstrtab         STRTAB          0000000000000000 000040 000011 00      0   0  1
> ...
> ```
> 
> If I remove `isRelocatable()` here, I can match that behavior.
> I don't think this part needs to be guarded by isRelocatable()

Relocation sections are typically linked to .symtab so you can't remove it even if it is empty. It is still possible to check object for relocation sections and then check those sections link field, but it seems like an overkill, IMO.


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

https://reviews.llvm.org/D61672