This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] [COFF] Implement --strip-debug
ClosedPublic

Authored by mstorsjo on Jan 17 2019, 3:24 AM.

Diff Detail

Event Timeline

mstorsjo created this revision.Jan 17 2019, 3:24 AM
jhenderson added inline comments.Jan 17 2019, 6:57 AM
test/tools/llvm-objcopy/COFF/strip-debug.test
1

Are you planning on using this yaml file for other tests? Otherwise, it seems confusingly named.

3

Nit, you're being inconsistent with using single and double-dashes for your options in this file. I prefer double, to avoid theoretical issues with grouping of single-letter options.

7

What is SECTIONS-POST for, here and below? I can't see any check patterns for it.

mstorsjo marked 4 inline comments as done.Jan 17 2019, 11:33 AM
mstorsjo added inline comments.
test/tools/llvm-objcopy/COFF/strip-debug.test
1

I meant to use it for multiple tests, but it was enough with one, as all these switches turned out to do the same for sections. Will fix.

mstorsjo updated this revision to Diff 182362.Jan 17 2019, 11:34 AM
mstorsjo marked an inline comment as done.

Fixed @jhenderson's suggestions.

jhenderson accepted this revision.Jan 18 2019, 4:32 AM

LGTM, assuming that this is the same behaviour as GNU objcopy.

This revision is now accepted and ready to land.Jan 18 2019, 4:32 AM
rupprecht added inline comments.Jan 18 2019, 2:25 PM
test/tools/llvm-objcopy/COFF/strip-debug.yaml
1 ↗(On Diff #182362)

Does this test run? I thought it had to end in ".test" to run.
https://github.com/llvm/llvm-project/blob/master/llvm/test/lit.cfg.py#L25

mstorsjo marked an inline comment as done.Jan 18 2019, 2:52 PM
mstorsjo added inline comments.
test/tools/llvm-objcopy/COFF/strip-debug.yaml
1 ↗(On Diff #182362)

Oh crap, you're right. I'm used to working on tests in e.g. lld, where .yaml already is added to that list of suffixes, and where the convention is that if the file actually contains yaml input data, it's named .yaml, otherwise .test.

I've already committed one test with a .yaml suffix though. Do you want me to add a lit.local.cfg that adds yaml to the list (kind of like https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/yaml2obj/lit.local.cfg), or should I rename the existing test to .test? (I didn't notice since I've been running the tests with llvm-lit path/to/llvm-objcopy/COFF/*.)

mstorsjo marked an inline comment as done.Jan 18 2019, 3:13 PM
mstorsjo added inline comments.
test/tools/llvm-objcopy/COFF/strip-debug.yaml
1 ↗(On Diff #182362)

I presume renaming to .test is easiest, and matches the ELF backend's tests, so I'll rename the existing test in one NFC commit and rename the newly added tests before I commit them as well.

rupprecht accepted this revision.Jan 18 2019, 3:41 PM

Accepted assuming the test case passes when renamed to .test :)

test/tools/llvm-objcopy/COFF/strip-debug.yaml
1 ↗(On Diff #182362)

SGTM -- I think just renaming these to .test is better.

I wouldn't strongly object to allowing .yaml as a test case extension if others are used to it, but it seems a little weird to me...

This revision was automatically updated to reflect the committed changes.