Page MenuHomePhabricator

llvm-objcopy: Add support for -g as an alias for --strip-debug
ClosedPublic

Authored by dyung on Jan 24 2019, 8:26 PM.

Details

Summary

This change adds an option -g to llvm-objcopy which is an alias for the existing option --strip-debug.

I also modified the existing tests which use the --strip-debug option to exercise the -g option as well.

This fixes PR40003.

Diff Detail

Repository
rL LLVM

Event Timeline

dyung created this revision.Jan 24 2019, 8:26 PM
alexshap added inline comments.Jan 24 2019, 8:44 PM
llvm/test/tools/llvm-objcopy/ELF/symtab-link.test
4 ↗(On Diff #183474)

what is this second run for ? appears to be exactly the same as the line 2

dyung added inline comments.Jan 24 2019, 8:50 PM
llvm/test/tools/llvm-objcopy/ELF/symtab-link.test
4 ↗(On Diff #183474)

Whoops, I don't know why I added those, it was unnecessary. I'll remove them. Thanks for catching that.

dyung updated this revision to Diff 183475.Jan 24 2019, 8:54 PM

Removed extra test lines that were not needed.

alexshap added inline comments.Jan 24 2019, 9:00 PM
test/tools/llvm-objcopy/COFF/only-section.test
13 ↗(On Diff #183475)

i don't have a strong opinion regarding this, but generally when the output is expected to be exactly the same i wouldn't do text matching twice (it all contributes to the time it takes to run all the tests), i'd simply run cmp %t.combination.exe %t.combination_g.exe - it's much cheaper.

p.s. maybe there are other similar places, pls, have a look.

Code lgtm, just have nits on the testing. Thanks!

test/tools/llvm-objcopy/ELF/help-message.test
14 ↗(On Diff #183475)

Not related to this patch really, but --strip-debug isn't needed for this test case -- the same error message should be printed with just llvm-strip (no args)

test/tools/llvm-objcopy/ELF/strip-debug.test
10 ↗(On Diff #183475)

This test -- running cmp against -g and --strip-debug output -- is really all that's needed. Duplication in all the other tests is not providing any more coverage. I think it's safe to revert all the other test files and all other modifications in this file.

dyung marked an inline comment as done.Jan 25 2019, 12:13 AM
dyung added inline comments.
test/tools/llvm-objcopy/ELF/strip-debug.test
10 ↗(On Diff #183475)

I agree. I'll remove the rest of the changes and update the diff.

I agree with Jordan regarding tests, otherwise this change looks good to me.

dyung updated this revision to Diff 183487.Jan 25 2019, 12:20 AM

Remove redundant testing and leave in one check that -g and --strip-debug produce identical output.

alexshap accepted this revision.Jan 25 2019, 12:47 AM
This revision is now accepted and ready to land.Jan 25 2019, 12:47 AM
This revision was automatically updated to reflect the committed changes.