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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
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);
----------------
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.


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


================
Comment at: tools/llvm-objcopy/Object.h:378
   void removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
+  void setSymbolInRelocByIndex(uint32_t Index);
 
----------------
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.


================
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 &&
----------------
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?


================
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) ||
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D46896