Page MenuHomePhabricator

[llvm-strip] Add support for -remove-section
ClosedPublic

Authored by alexshap on May 7 2018, 5:35 PM.

Details

Summary

This diff adds support for -remove-section.
Test plan: make check-all

Diff Detail

Repository
rL LLVM

Event Timeline

alexshap created this revision.May 7 2018, 5:35 PM
jakehehrlich added inline comments.May 7 2018, 6:06 PM
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145605)

Why not just extend the existing --remove-section tests with llvm-strip?

alexshap added inline comments.May 7 2018, 6:16 PM
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145605)

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.

jhenderson added inline comments.May 8 2018, 1:44 AM
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145605)

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

alexshap added inline comments.May 8 2018, 8:39 AM
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145605)

oh, right, will do

alexshap added inline comments.May 8 2018, 11:00 AM
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145605)

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)

jakehehrlich added inline comments.May 8 2018, 12:08 PM
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145605)

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.

alexshap added inline comments.May 8 2018, 12:39 PM
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145605)

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 ?

jakehehrlich added inline comments.May 8 2018, 12:51 PM
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145605)

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

alexshap updated this revision to Diff 145774.May 8 2018, 1:56 PM

Address comments

alexshap updated this revision to Diff 145775.May 8 2018, 1:57 PM

Remove blank line

jakehehrlich added inline comments.May 8 2018, 2:01 PM
test/tools/llvm-objcopy/strip-and-remove.test
1–2 ↗(On Diff #145774)

I'd prefer not regenerating the elf. You don't even need to run the FileCheck again. Principally this is an llvm-strip test and we just want to add a couple lines to make sure the equivlient llvm-objcopy produces an exactly equivalent binary. It is assumed that if the binaries are equivalent they will produce equivalent results.

alexshap updated this revision to Diff 145783.May 8 2018, 2:13 PM

generate elf only once

jakehehrlich accepted this revision.May 8 2018, 2:19 PM

LGTM: Please wait on James as I'd like his input on how we should test this sort of stuff going forward as well.

test/tools/llvm-objcopy/remove-section.test
2 ↗(On Diff #145784)

nit: I'd prefer not to copy in this case

This revision is now accepted and ready to land.May 8 2018, 2:19 PM
alexshap added inline comments.May 8 2018, 2:22 PM
test/tools/llvm-objcopy/remove-section.test
2 ↗(On Diff #145784)

i copied because potentially llvm-objcopy can modify the input (i.e. by mistake),
it's not the case here, but my motivation was to reduce the room for potential interfering of different runs.

jakehehrlich added inline comments.May 8 2018, 2:32 PM
test/tools/llvm-objcopy/remove-section.test
2 ↗(On Diff #145784)

Ah, in that case can you add a check after the first objcopy to check that %t and %t1 are binary equivalent after the first call to objcopy? That way we can detect that case if it occurs.

alexshap updated this revision to Diff 145793.May 8 2018, 2:54 PM

Add one more "defensive" cmp

rja accepted this revision.May 8 2018, 3:52 PM
rja added a subscriber: rja.

Add one more "defensive" cmp

LG

jhenderson accepted this revision.May 9 2018, 2:13 AM

The overall approach looks fine to me. I think I agree with your comments about not overloading remove-section.test. I think your solution is actually better in this case. We don't have much combination testing of switches in llvm-objcopy, if I remember correctly, so having distinct llvm-strip tests where we are effectively testing some new combo is reasonable, in my opinion, but as noted by @jakehehrlich, we should definitely check the equivalent llvm-objcopy equivalent in the same test. Essentially if we can do some invocation of llvm-objcopy and some (potentially different) invocation of llvm-strip and get the same output in both cases, there should be a test containing them both. If the behaviour cannot be replicated by llvm-strip, then we should only test llvm-objcopy. That being said, I'm not opposed to a --no-strip switch, although I'm not as certain of the value.

LGTM, with lots of small comments, though I'm not too bothered by any of them, so if you disagree with any, feel free to go ahead and commit anyway.

test/tools/llvm-objcopy/remove-section.test
1 ↗(On Diff #145793)

No issues with the changes made in this test, but since this isn't testing llvm-strip, I'd prefer it if they were in a separate commit. Feel free to commit it without further review.

6 ↗(On Diff #145793)

Probably worth commenting why we do a check here. There are probably several other cases in other tests where we do something similar too, which should probably be updated.

test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145793)

What are your thoughts on renaming this test to strip-all-and-remove.test? That way it's clear that we are testing the combination of the two switches of llvm-objcopy, one of which is effectively always on for llvm-strip. It also allows for easier distinguishing from strip-debug.

17 ↗(On Diff #145793)

If this is intended to be a real debug section, it probably is worth naming it in the same style as other debug sections i.e. .debug_foo.

44 ↗(On Diff #145793)

Should we have a CHECK-NOT: Name: .symtab here?

Strictly speaking, an empty symbol table is distinct from a missing one.

test/tools/llvm-objcopy/strip-debug-and-remove.test
17 ↗(On Diff #145793)

See my comments in the other test.

alexshap marked an inline comment as done.May 10 2018, 9:35 PM
alexshap added inline comments.
test/tools/llvm-objcopy/remove-section.test
1 ↗(On Diff #145793)

r332078

alexshap marked an inline comment as done.May 10 2018, 9:57 PM
alexshap added inline comments.
test/tools/llvm-objcopy/strip-and-remove.test
1 ↗(On Diff #145793)

yeah, sounds good to me, will rename this test to strip-all-and-remove.test

17 ↗(On Diff #145793)

ok, will rename it to .debug_foo

alexshap added inline comments.May 10 2018, 10:27 PM
test/tools/llvm-objcopy/strip-and-remove.test
44 ↗(On Diff #145793)

yeah

This revision was automatically updated to reflect the committed changes.