This is an archive of the discontinued LLVM Phabricator instance.

[llvm] remove empty __LLVM segment in llvm-bitcode-strip
ClosedPublic

Authored by rmaz on Mar 3 2022, 11:26 AM.

Details

Summary

When running llvm-bitcode-strip we want to remove the __LLVM
segment as well as the __bundle section when there are no other
sections in the segment.

Diff Detail

Event Timeline

rmaz created this revision.Mar 3 2022, 11:26 AM
rmaz requested review of this revision.Mar 3 2022, 11:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 11:26 AM
rmaz updated this revision to Diff 412785.Mar 3 2022, 11:28 AM

update test comment

rmaz edited the summary of this revision. (Show Details)Mar 3 2022, 11:29 AM
jhenderson added inline comments.Mar 4 2022, 12:39 AM
llvm/test/tools/llvm-objcopy/MachO/bitcode-strip-remove-nonempty-segment.test
4–6

Ideally, we'd use the same invocation to check everything at once. I believe llvm-readobj has an option to dump Mach-O segments. Does that suffice?

llvm/test/tools/llvm-objcopy/MachO/bitcode-strip-remove.test
11

It's usually a bad idea to have a purely negative check like this, because they can rot too easily. Better would be to check all segments and show the one you want isn't in that set.

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

its - belonging to it
it's - it is

rmaz updated this revision to Diff 413153.Mar 4 2022, 3:07 PM

Single pass tests

rmaz updated this revision to Diff 413154.Mar 4 2022, 3:14 PM
rmaz marked an inline comment as done.

Use llvm-readobj --macho-segment

rmaz marked 2 inline comments as done.Mar 4 2022, 3:17 PM
jhenderson accepted this revision.Mar 6 2022, 10:47 PM

LGTM, with one change.

By the way, if you haven't already, you've probably done enough to request commit access.

llvm/test/tools/llvm-objcopy/MachO/bitcode-strip-remove.test
14

No need for this line: 1) the --implicit-check-not covers it. 2) This won't catch __LLVM appearing before or between the other segments.

This revision is now accepted and ready to land.Mar 6 2022, 10:47 PM
rmaz updated this revision to Diff 413458.Mar 7 2022, 7:08 AM

Remove unnecessary CHECK-NOT

rmaz updated this revision to Diff 413459.Mar 7 2022, 7:09 AM

With linter

rmaz marked an inline comment as done.Mar 7 2022, 7:12 AM
jhenderson accepted this revision.Mar 7 2022, 7:30 AM
This revision was landed with ongoing or failed builds.Mar 7 2022, 8:52 AM
This revision was automatically updated to reflect the committed changes.