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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
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;
----------------
rupprecht wrote:
> 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.
In case I wasn't specific enough: by "refactor this" I mean just the `(Obj.Type == ET_EXEC || Obj.Type == ET_DYN)` check.


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

https://reviews.llvm.org/D61672