[PATCH] D46896: [llvm-objcopy] Add --strip-unneeded option

[Paul Semel via Phabricator via llvm-commits]

paulsemel added a comment.

Hi !

I kind of disagree with one point. If we chose to not differentiate between ET_* files, we need to keep Global/Weak defined symbol in every cases.
Indeed, it just doesn't make sense to strip thoses symbols for ET_REL files, as it will make the relocatable file not usable.. (or kind of not usable).
I actually don't care for the remaining part (even if I admit I like `objcopy` current behavior).
I will wait for you to tell me what to do, I don't want to take decisions you will not approve :)



================
Comment at: tools/llvm-objcopy/Object.h:347
   uint8_t Visibility;
+  bool InReloc;
 
----------------
jakehehrlich wrote:
> paulsemel wrote:
> > jakehehrlich wrote:
> > > I'm not sure this is sufficient, or if it is it has to be set at the right time. For example say a symbol is referenced by a relocation in .rel.data and then .rel.data is removed. It could be that that symbol is either still referenced by another relocation in another section or it could not be. No local operation would tell you the answer either.
> > > 
> > > I think there are two possibilities.
> > > 1) Maintain a count that can be incremented/decremented as relocations are added/removed
> > > 2) Do a pass at the end to determine which symbols should be kept/not kept. You can either set this bool in that pass or add symbols to a set like container. see: http://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc
> > Arf.. I totally forgot the "what if the reloc section is removed" part Thanks about it !
> > I think I will go for the counter, as I first wanted to be as light as possible for this option (and I don't see anything that might cause problems with the counter).
> I was thinking about this and I was wondering if this isn't something that should be more general. Like it there are other ways symbols might be needed other than relocations. This should be a generic reference count. But we already have shared_ptr and weak_ptr to tell us how this should work! 
> 
> Can we think of a way to reuse that infrastructure? For instance those types already have use_count methods so that can be used to determine weather a symbol is needed or not. Alternatively it would make sense to create a similar sort of abstraction that managed the reference counting for us.
Hm.. ok do you already have something in mind ?
The fact is that not only relocation can reference symbols, so we need to be able to distinguish between blocking references and normal references if you know what I mean. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:392
 
+      if (Config.StripUnneeded && !Sym.InReloc && Sym.Index != 0 &&
+          ((Obj.Type == ET_REL && Sym.Binding == STB_LOCAL &&
----------------
jakehehrlich wrote:
> paulsemel wrote:
> > jakehehrlich wrote:
> > > The whole Sym.Index != 0 smells to me and is a product of me not thinking about the dummy symbol. At the very least that should never be past to a function in removeSymbols or updateSymbols. It might be better to handle the dummy symbol the way we handle the dummy section, namely not store it and account for the increased index. Probably best for another change though. Could you add a TODO here for that?
> > I totally agree with you ! I Actually wanted to talk with you about this dummy symbol, because there is some problems, especially with sections where the only remaining symbol is the dummy one (we might want to remove the section).
> > I will add a TODO, no problem ! :)
> We probably don't want to remove empty symbol tables IMO. I don't think it has any real tendency to come up for any practical purpose where the desire wouldn't just be to remove the section directly.
I don't get it. What's the point of keeping an empty symtab ?


Repository:
  rL LLVM

https://reviews.llvm.org/D46896