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

[Paul Semel via Phabricator via llvm-commits]

paulsemel added a comment.

I'm sorry guys, but I think I am missing something. Indeed, if I used the ELF type and not something else, it's because objcopy is keeping global and weak symbols (except for undefined ones) for relocatable files.
This is actually done even if the symbol is not referenced by a relocation. That's why I don't really see what I can do with `SHF_ALLOC`.
Can you elaborate on this one ?



================
Comment at: tools/llvm-objcopy/Object.cpp:653-659
     Relocation ToAdd;
     ToAdd.Offset = Rel.r_offset;
     getAddend(ToAdd.Addend, Rel);
     ToAdd.Type = Rel.getType(false);
     ToAdd.RelocSymbol = SymbolTable->getSymbolByIndex(Rel.getSymbol(false));
+    SymbolTable->setSymbolInRelocByIndex(Rel.getSymbol(false));
     Relocs->addRelocation(ToAdd);
----------------
jakehehrlich wrote:
> I don't nessarilly think this should be done in this change but I think I don't like how I wrote addRelocation. I think it should take the parameters to build a relocation not request that the user build the relocation. That would  have the added benefit of allowing addRelocation to update the aforementioned count.
Well, maybe it can be a bit confusing to do this "fix" in this change.. I can add a TODO if you prefere ?


================
Comment at: tools/llvm-objcopy/Object.h:347
   uint8_t Visibility;
+  bool InReloc;
 
----------------
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).


================
Comment at: tools/llvm-objcopy/Object.h:378
   void removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
+  void setSymbolInRelocByIndex(uint32_t Index);
 
----------------
jakehehrlich wrote:
> I don't really like this method. It's really specific and most of the overly specific methods I've implemented in objcopy have needed to be generalized later or caused some sort of issue. In this case I think just adding a non-const version of getSymbolByIndex is sufficient and more general.
Ok, sounds fair !


================
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:
> 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 ! :)


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:393
+      if (Config.StripUnneeded && !Sym.InReloc && Sym.Index != 0 &&
+          ((Obj.Type == ET_REL && Sym.Binding == STB_LOCAL &&
+            Sym.Type != STT_FILE && Sym.Type != STT_SECTION) ||
----------------
jakehehrlich wrote:
> Did you observe that GNU objcopy only worked on ET_REL? Also did you observe GNU objcopy not use this for locals? I think it makes perfect sense to remove undefined globals/weaks not used in a relocation.
You're right, I missed this too..
It actually removes global/weak symbols that are undefined and not used in relocation. I'm going to change this to fit the `objcopy` behavior, thanks for spotting this !


Repository:
  rL LLVM

https://reviews.llvm.org/D46896