This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][MachO] Implement --remove-section
ClosedPublic

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

Event Timeline

seiya created this revision.Aug 15 2019, 2:38 AM
jhenderson added inline comments.Aug 15 2019, 3:35 AM
llvm/test/tools/llvm-objcopy/MachO/remove-section.test
2

in -> by

This test should also show that you can remove multiple sections at the same time with the switch.

LGTM, no concerns from me once the test is updated

seiya updated this revision to Diff 215524.Aug 15 2019, 7:06 PM
seiya marked an inline comment as done.
  • Addressed review comments.
seiya updated this revision to Diff 215811.Aug 18 2019, 9:13 PM
  • Added a comment in the test.
jhenderson added inline comments.Aug 19 2019, 3:04 AM
llvm/test/tools/llvm-objcopy/MachO/remove-section.test
7

"Error case 1:" is meaningless without other error cases.

63

How easy would it be to use FileCheck --check-prefixes, and possibly -D<NAME> options to share this block with the previous test case? That would avoid duplicating a big chunk of the expected output. You could do something like:

# RUN: ... | FileCheck --check-prefixes=REMOVE-COMMON,REMOVE-TEXT-ONLY
# RUN: ... | FileCheck --check-prefixes=REMOVE-COMMON

# REMOVE-COMMON:
# REMOVE-COMMON-NEXT:
...
# REMOVE-TEXT-ONLY-NEXT:
# REMOVE-TEXT-ONLY-NEXT:
...
# REMOVE-COMMON-NEXT:
seiya marked 2 inline comments as done.Aug 19 2019, 2:44 PM
seiya updated this revision to Diff 215989.Aug 19 2019, 2:44 PM

Address review comments.

jhenderson accepted this revision.Aug 20 2019, 2:11 AM

LGTM, thanks.

This revision is now accepted and ready to land.Aug 20 2019, 2:11 AM
seiya updated this revision to Diff 216120.Aug 20 2019, 6:06 AM

Update CommandGuide.

jhenderson accepted this revision.Aug 20 2019, 6:16 AM
seiya updated this revision to Diff 216129.Aug 20 2019, 6:24 AM

CommandGuide: Removed remove "the" before "<section>".

jhenderson added inline comments.Aug 20 2019, 6:24 AM
llvm/docs/CommandGuide/llvm-objcopy.rst
84 ↗(On Diff #216120)

Delete "the"

seiya marked an inline comment as done.Aug 20 2019, 6:26 AM
seiya updated this revision to Diff 223727.Oct 7 2019, 8:30 PM

Rebased.

rupprecht accepted this revision.Oct 8 2019, 10:57 AM
rupprecht added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
42–46

Not related to this patch, but looks like this discards RemovePred, e.g. --strip-all --only-section will effectively work like --only-section.

I found this after noticing that RemovePred is not referenced in the bit you added, which is actually fine there but error prone if the iterative construction of RemovePred is ever reordered. ELF objcopy works like this too, but correctly chains RemovePred for the --only-section switch.

rupprecht marked an inline comment as done.Oct 8 2019, 11:27 AM
rupprecht added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
42–46

Nevermind, that flag takes priority, so that's WAI. Ignore this comment.

seiya marked an inline comment as done.Oct 9 2019, 12:15 AM
seiya added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
42–46

I'll add a comment for it. It's worth mentioning in the code.

This revision was automatically updated to reflect the committed changes.