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

Jake Ehrlich via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 8 12:51:56 PDT 2018


jakehehrlich 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:
> jakehehrlich wrote:
> > alexshap wrote:
> > > 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)
> > So thinking about this, the complication seems to be that -R dosn't disable --strip-all so it dosn't fit neatly into the existing test. In light of that I don't suppose it's all that easy to add that functionality. That makes me think we might want a --no-strip option for testing's sake since we want to add lots of 'cmp' checks to our tests to make sure llvm-strip and llvm-objcopy never differ by even a byte.
> > 
> > I don't care much about dedupping yaml TBH but I do care about test failures being as meaningful as possible (which means airing on the side of not merging). I do care about testing that llvm-objcopy and llvm-strip are binary equivalent however. I'd be satisfied with just adding cmp checks to these existing checks. I think the llvm-objcopy invocations will also clarify a) why this test is separate and b) will document the semantics of llvm-strip.
> > 
> > Also adding both short and long versions (as James recommended) and running cmp to make sure those are the same would be nice. Basically if there are multiple ways of doing the *exact* same thing I want to test all those to make sure they are in fact exactly the same to ensure that nothing ever diverges.
> sorry, not sure i understand why we would need a new flag "--no-strip", maybe i'm missing smth though, as i said above - to test strip the input (to make the test meaningful) should have debug info and symbols, the existing test remove-section.test at the moment contains neither of them (debug info nor symbols) and I'm hesitating to load that particular test with complications.
> 
> For example, in strip-debug-and-remove.test:
>     llvm-strip --strip-debug -remove-section=.text.bar a.o
> is equivalent to
>     llvm-objcopy --strip-debug -remove-section=.text.bar b.o b.o
>     
>     cmp a.o b.o
> 
> i can add objcopy /cmp invocations to these tests - is that what you are saying / sounds good or there are some other concerns i've missed ?
> 
> 
--no-strip would be an option for llvm-strip that disabled --strip-all without enabling another strip option. I don't think it should be added in this test and I don't think you should touch remove-sections.test. It's just an option I might add for testing.

As for the second part, yes, that's what I mean and that covers my concerns.


Repository:
  rL LLVM

https://reviews.llvm.org/D46567





More information about the llvm-commits mailing list