This is an archive of the discontinued LLVM Phabricator instance.

[llvm-strip] Support grouped options in llvm-strip
ClosedPublic

Authored by drodriguez on Jun 30 2021, 4:57 PM.

Details

Summary

GNU and Apple strip implementations seems to support grouped options.
Enable the support for grouped options introduced in
https://reviews.llvm.org/D83639 for llvm-strip invocations.

Includes test that checks that both the grouped and non grouped
invocations produces the same result.

Diff Detail

Event Timeline

drodriguez created this revision.Jun 30 2021, 4:57 PM
drodriguez requested review of this revision.Jun 30 2021, 4:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2021, 4:57 PM
MaskRay added inline comments.Jun 30 2021, 5:38 PM
llvm/test/tools/llvm-objcopy/grouped-options.test
17

You may drop .zdebug and .gdb_index . There is a main test checking the functionality, no need to replicate that here.

Remove .zdebugGlobal and .gdb_index from the test

drodriguez marked an inline comment as done.Jun 30 2021, 7:03 PM
This revision is now accepted and ready to land.Jun 30 2021, 7:16 PM

I don't think it should make a difference if the class is only instantiated in one place, but other parts of the codebase seem to do this in the class constructor, e.g. https://github.com/llvm/llvm-project/blob/3afbf898044aa5839ed75273fa38a897abe9d3d4/llvm/tools/llvm-objdump/llvm-objdump.cpp#L97.

Does it make sense to do this for llvm-objcopy at the same time? My quick testing suggests GNU objcopy accepts grouped options, but llvm-objcopy doesn't, at least for the version I've got on my machine.

llvm/test/tools/llvm-objcopy/grouped-options.test
2

Add a brief top-level comment to the test explaining the purpose of the test.

48

Nit: delete the extra trailing blank line.

drodriguez updated this revision to Diff 355922.Jul 1 2021, 9:44 AM

Move configuration to constructors. Enable grouped options in llvm-objcopy. Add comment about the intention of the test. Remove blank line.

drodriguez marked 2 inline comments as done.Jul 1 2021, 9:45 AM
MaskRay accepted this revision.Jul 1 2021, 11:14 AM

Thanks!

This revision was landed with ongoing or failed builds.Jul 1 2021, 1:37 PM
This revision was automatically updated to reflect the committed changes.
jhenderson added inline comments.Jul 2 2021, 12:03 AM
llvm/test/tools/llvm-objcopy/grouped-options.test
2–3

Up to you whether you bother fixing it now, but in the newer tests in the LLVM binutils, we tend to use ## for comments, to help distinguish them from RUN and CHECK directives.

drodriguez added inline comments.Jul 6 2021, 9:09 AM
llvm/test/tools/llvm-objcopy/grouped-options.test
2–3

Thanks for the tip! I didn't noticed the pattern before. If I modify this file (or others) again, I will try to remember.