[PATCH] D39769: [llvm-objcopy] Add --strip-all option to llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D39769#922369, @jakehehrlich wrote:

> Ok I think this should just follow what GNU objcopy does for mass consistency but I'm also going to add --only-keep-allocated to do what I intended this to do in the first place


That sounds like a reasonable plan. I'm a big fan of drop-in replacements, as long as it doesn't particularly harm the interface, because it makes migration a lot easier. I could imagine a case where somebody expects .comment to be kept, but if it isn't, a breakage might occur somewhere down a potentially very long build pipeline.

> 1. I couldn't add dynamic relocations because yaml2obj will only give a relocation section a link for a non-dynamic symbol table and llvm-objcopy throws an error if you give it a dynamic relocation section without a dynamic symbol table. I added every other section requested though.

I find it slightly surprising that yaml2obj automatically assumes relocation sections with SHF_ALLOC to be dynamic relocation sections, given we're working with an ELF of ET_REL type. Given that though, this is okay.

LGTM.



================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:177
+      if ((Sec.Flags & SHF_ALLOC) != 0)
+        return false;
+      if (&Sec == Obj->getSectionHeaderStrTab())
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Won't this and the next "return false" override any other "RemovePred" specified earlier? They should probably be "return RemovePred(Sec)" or similar, otherwise you can't do "--strip-all --remove-section=.text" or similar.
> Yea this is a problem. This is not a consistent way to behave.
> 
> There's a general problem here of how we should resolve disagreements between predicates. So a predicate should have 3 stances on the removal of a section a) remove b) keep or c) don't care. option c) can't cause a contradiction but a) and b) can conflict if used. We could take a general stance and say that only a) and c) are allowed then rather than returning "false" in any given case we just return the result of the previous RemovePred. That actually sounds the most sensible to me. Order of arguments wouldn't matter if we adopt that stance. For sections there are not "but keep this" operations so removals should compose I think.
> 
> The only other option I can see is to make order matter and to process the options by their order. I'm generally in favor of order not mattering however so I think I should replace any "return false" with "return RemovePred(...)" instead. I don't really strongly hold that opinion however so I would be happy doing things in a specific order as well. This code as it stands is order agnostic but resolves contradictions in a specific and bad way. It should either a) now have flags that contradict each other or b) allow later flags to overrule earlier flags. I'm in favor of a) unless GNU objcopy does b)
I prefer the "order doesn't matter" approach for now. We might have to be careful when we add any "keep" options though, e.g. what happens if you do -R .text -j .text? Is it the same as -j .text -R .text? Something to poke GNU objcopy with, I think. If we're lucky GNU treats one or the other as higher priority, ignoring order. If we're unlucky, we're going to need to completely change this area of code to be some kind of argument dispatching mechanism.


Repository:
  rL LLVM

https://reviews.llvm.org/D39769