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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> The point is to test that dynamic symbols are not touched by --strip-unneeded run on executable. Do you think it's not needed?

It's not needed. We don't do anything in llvm-objcopy to the dynamic symbols ever, so I don't think we need to test this in this specific case.



================
Comment at: test/tools/llvm-objcopy/ELF/no-symbol-relocation.test:9
   Data:            ELFDATA2LSB
-  Type:            ET_EXEC
+  Type:            ET_REL
   Machine:         EM_X86_64
----------------
Why are you changing this?


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded-all-symbols.test:2
+## Stripping unneeded symbols from execuatble/DSO should
+## eliminate static symbol table, because it's not used
+## by dynamic loader. 
----------------
eliminate the static


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded-all-symbols.test:3
+## eliminate static symbol table, because it's not used
+## by dynamic loader. 
+
----------------
by the dynamic loader.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded-all-symbols.test:17-20
+DynamicSymbols:
+  - Name:     foo
+    Type:     STT_FUNC
+    Binding:  STB_GLOBAL
----------------
As noted in an earlier diff, dynamic symbols aren't related to this change, so can be deleted (see out-of-line comment).


================
Comment at: test/tools/llvm-objcopy/ELF/strip-unneeded-all-symbols.test:23
+  - Name:     bar
+    Type:     STT_FUNC
+  - Name:     foo
----------------
The type is irrelevant to the behaviour, so it can be deleted.


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:424
          is_contained(Config.UnneededSymbolsToRemove, Sym.Name)) &&
-        isUnneededSymbol(Sym))
+        (!Obj.isRelocatable() || isUnneededSymbol(Sym)))
       return true;
----------------
Why is this here? Is it tested?


================
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())
----------------
evgeny777 wrote:
> 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.
Probably worth a comment in the code to this effect. I don't feel strongly like we need to drop symbol tables in relocatable files.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1591
+Error Object::removeUnneededSections() {
+  // We can remove trivial symbol table from non-relocatable objects
+  if (isRelocatable() || SymbolTable == nullptr || !SymbolTable->isTrivial())
----------------
jhenderson wrote:
> Missing trailing full stop.
> "remove trivial" -> "remove an empty".
Ping here.


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

https://reviews.llvm.org/D61672