[PATCH] D46567: [llvm-strip] Add support for -remove-section

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: test/tools/llvm-objcopy/strip-and-remove.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-strip -remove-section=.text.bar %t
----------------
alexshap wrote:
> jhenderson wrote:
> > alexshap wrote:
> > > jakehehrlich wrote:
> > > > Why not just extend the existing --remove-section tests with llvm-strip?
> > > the existing tests 
> > > 
> > > alexshap-mbp:llvm-objcopy alexshap$ grep -n remove-section ./*
> > > 
> > > ./armexidx-link.test:2:# RUN: llvm-objcopy -remove-section=.text.bar %t %t2
> > > ./group-big-endian.test:2:# RUN: llvm-objcopy -remove-section=.text.bar %t %t2
> > > ./group-unchanged.test:2:# RUN: llvm-objcopy -remove-section=.text.bar %t %t2
> > > ./group.test:2:# RUN: llvm-objcopy -remove-section=.text.bar %t %t2
> > > 
> > > those tests verify something else and the inputs don't contain debug info.
> > > At the beginning i thought i would extend one of them, 
> > > but then it turned out to be less straightforward than I expected or I don't see the test you are referring to.
> > > 
> > > 
> > There are a few tests like remove-section.test that only use the -R short option. It might be better to extend those (and probably also test both short and long options to avoid this confusion).
> oh, right, will do
right now I'm looking at remove-section.test - i can try to extend (essentially replace) that test, but to be honest I'm not 100% sure it's a good idea since that test was probably designed to be simple and verify -remove-section basic functionality only
(in particular, it doesn't have debuginfo, symbols etc).

@jakehehrlich - what are your thoughts - are you sure it's a good idea to load that test with all this stuff ? 

looking at my diff - it seems to me that it would be better to merge strip-and-remove.test and strip-debug-and-remove.test since they share the "input", what would you say to this approach ?  (either way, i don't have a strong opinion on this)


Repository:
  rL LLVM

https://reviews.llvm.org/D46567