This is an archive of the discontinued LLVM Phabricator instance.

[llvm] add -o flag to llvm-bitcode-strip
ClosedPublic

Authored by rmaz on Mar 1 2022, 7:27 AM.

Details

Summary

Add the -o flag to specify an output path for llvm-bitcode-strip.
This matches the interface to the Xcode bitcode_strip tool.

Diff Detail

Event Timeline

rmaz created this revision.Mar 1 2022, 7:27 AM
rmaz requested review of this revision.Mar 1 2022, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 7:27 AM

I was going to suggest that the option should be documented in the llvm-bitcode-strip command guide, but it looks like that this documentation hasn't been written yet :(

llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test
2
5

You shouldn't just run a naked not ... without checking the error message produced by llvm-bitcode-strip

6

You should check that %t2 contains the output you expect, whatever that may be.

30

I suspect you only need 0 or 1 sections for the test case to be sufficient?

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
1211

Are you sure you want this behaviour? Prior to this patch, llvm-bitcode-strip will modify things in-place, like llvm-strip does.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 10:06 PM
rmaz added inline comments.Mar 2 2022, 7:13 AM
llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
1211

Yes, this matches the behaviour of bitcode_strip:

Usage: bitcode_strip input [-r | -m | -l] [-keep_cs] -o output
rmaz updated this revision to Diff 412413.Mar 2 2022, 7:29 AM

update test to check error message, remove unused sections and verify output is unchanged.

rmaz marked 4 inline comments as done.Mar 2 2022, 7:30 AM
jhenderson added inline comments.Mar 2 2022, 10:36 PM
llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
1214

I'd change this comment: the error message already has llvm-bitcode-strip in the prefix, so no need to repeat it in the body of the message. Perhaps simply "-o is a required argument" (alternative ideas also welcome)?

rmaz updated this revision to Diff 412716.Mar 3 2022, 7:27 AM

reword error message

rmaz marked an inline comment as done.Mar 3 2022, 7:28 AM
rmaz updated this revision to Diff 412718.Mar 3 2022, 7:34 AM

update test

This revision is now accepted and ready to land.Mar 4 2022, 12:14 AM
This revision was landed with ongoing or failed builds.Mar 4 2022, 8:04 AM
This revision was automatically updated to reflect the committed changes.