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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

In D61672#1496975 <https://reviews.llvm.org/D61672#1496975>, @jakehehrlich wrote:

> In that case why not strip the symbol table all togethor?


GNU strip does this. In a really simple test -- build a dummy "int main() { return 0; }" executable, then run strip/llvm-strip --strip-unneeded -- GNU strip removes .symtab and .strtab, whereas llvm-strip does not. The .symtab/.strtab sections are trivial (only containing the null symbol), so we should do the final step and remove it. (Both leave in .shstrtab though).

So if we're going for better GNU strip emulation, this patch doesn't even go far enough, although it is a step in the right direction.



================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:408
          is_contained(Config.UnneededSymbolsToRemove, Sym.Name)) &&
-        isUnneededSymbol(Sym))
+        (Obj.Type == ET_EXEC || Obj.Type == ET_DYN || isUnneededSymbol(Sym)))
       return true;
----------------
evgeny777 wrote:
> jhenderson wrote:
> > What does GNU objcopy do for other ET_* types, e.g. ET_CORE or ET_* values in the OS and PROC-specific ranges? Could this be inverted to != ET_REL?
> May be. GNU objcopy considers everything which is not ET_DYN or ET_EXEC relocatable. See filter_symbols in objcopy.c
> 
> ```
>   int relocatable = (abfd->flags & (EXEC_P | DYNAMIC)) == 0;
> ```
> 
> 
Seconding what James said, I think we should refactor this out to a separate method. That way, any downstream users using llvm-objcopy with custom object file types that should be considered relocatable have a narrow/well defined place to put their local modifications and avoid merge conflicts any time this part is changed.

Ideally, this `Object` would be the one from libObject (`ELFObjectFile<ELFT>::isRelocatableObject()` in llvm/Object/ELFObjectFile.h), which I'm sure downstream users already have a local change for. Sadly, it's not, but downstream users can just add a second local patch.

I'm fine with either way, slightly preferring the way you have it here just because it probably emulates GNU objcopy better, but checking Type != ET_REL should also work.


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

https://reviews.llvm.org/D61672