Page MenuHomePhabricator

[llvm-objcopy][MachO] Implement --strip-all
AcceptedPublic

Authored by seiya on Aug 15 2019, 2:37 AM.

Event Timeline

seiya created this revision.Aug 15 2019, 2:37 AM
jhenderson added inline comments.Aug 15 2019, 3:39 AM
llvm/test/tools/llvm-objcopy/MachO/strip-all.test
2

You need a comment at the start of this test explaining what the test does.

13–14

Also llvm-strip without "--strip-all" should be the same as "llvm-objcopy--strip-all". Test that too.

44

No new line at EOF.

rupprecht added inline comments.Aug 15 2019, 2:28 PM
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
49

Can you skip the const_cast here and add a mutable getSymbolByIndex overload so you can remove the const modifiers elsewhere?

53–57

For ELF files, --strip-all seems to strip all symbols regardless of whether they're referenced by relocations, is that not true for MachO files?

102

Marking symbols should be behind whatever flags require it, so we don't needlessly traverse symbols

seiya updated this revision to Diff 215833.Aug 19 2019, 1:45 AM
seiya marked 7 inline comments as done.

Addressed review comments.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
53–57

We can't remove all symbols. Symbols referenced in the indirect symbol table are necessary for dynamic linking (i.e., there're no separate sections just like .dynsym and .dynstr).

Test case looks fine to me now. Not looked at the code though.

This revision is now accepted and ready to land.Tue, Sep 3, 3:14 PM

@seyia, sorry about the delays with code review, does this particular diff have any unreviewed dependencies ?

seiya added a comment.Tue, Sep 3, 7:02 PM

@seyia, sorry about the delays with code review, does this particular diff have any unreviewed dependencies ?

Thank you for the review! Currently, this patch depends on D66280 and D65541. I'll re-upload the latter one after refactoring CopyConfig to allow Mach-O specific command-line parsing.