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

[Paul Semel via Phabricator via llvm-commits]

paulsemel added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:267
 
+    if (Config.StripUnneeded) {
+      for (auto &Section : Obj.sections())
----------------
alexshap wrote:
> jakehehrlich wrote:
> > nit: If you could add a comment explaining what this is doing and why it needs to be here that would be great.
> @paulsemel, wait,  but shouldn't it be right after Obj.removeSections(RemovePred); ?
> (and in that case update the symbols table again)
> For example, if one removes a group of sections (including the .group section itself),
> the list of "referenced" symbols might potentially change, it's necessary to add a test for this case too, i think.
> 
> 
> 
The problem you are mentioning is actually a bigger one I told you about in your last review... (https://reviews.llvm.org/D47052)

Indeed, you introduced the whole behavior. This behavior is now the same as objcopy's. It seems that they are first removing symbols, and then sections, which in this case remove the group section but not the symbol. As far as I recall, I gave the example of removing a relocation and in the same time remove the referenced symbols (try it with objcopy and please read the comment I'm referring to). This is the exact same problem.

As I said in the mentioned comment, I still think we shouldn't have handled options conflicts this way, but anyway, if we want to handle all of those problems differently, I'm convinced this is not about this patch, but (again) more generally about how we handle option conflicts.


Repository:
  rL LLVM

https://reviews.llvm.org/D46896