Page MenuHomePhabricator

[llvm-objcopy][MachO] Make --remove-section clean up dead symbols
ClosedPublic

Authored by alexshap on Apr 19 2020, 10:59 PM.

Details

Summary

Make --remove-section clean up dead symbols, return an Error if it can't be safely done.

Test plan: make check-all

Diff Detail

Event Timeline

alexshap created this revision.Apr 19 2020, 10:59 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: abrachet. · View Herald Transcript
smeenai added inline comments.Apr 19 2020, 11:41 PM
llvm/tools/llvm-objcopy/MachO/Object.cpp
31

This is gonna reorder the actual section load commands, right? Is that okay?

44

Would SmallPtrSet be better?

jhenderson added inline comments.Apr 20 2020, 12:44 AM
llvm/test/tools/llvm-objcopy/MachO/remove-section-dead-symbols.test
5

What's the reason for dumping the sections here?

llvm/test/tools/llvm-objcopy/MachO/remove-section-error.test
3

You can probably use /dev/null as the output file here.

99

I know that mach-o yaml2obj requires very noisy output, but I'm guessing you don't need eh_frame data at least? Similarly, is there anything else you can remove?

llvm/tools/llvm-objcopy/MachO/Object.cpp
1

Could you fix the missing licence header please? Can be a separate change, no review needed.

alexshap marked an inline comment as done.Apr 20 2020, 1:28 AM
alexshap added inline comments.
llvm/test/tools/llvm-objcopy/MachO/remove-section-dead-symbols.test
5

we have removed one section ("C") - thus we'd like to verify that the list of sections is correct
["text", "data"]

alexshap marked 2 inline comments as done.Apr 20 2020, 1:36 AM
alexshap added inline comments.
llvm/test/tools/llvm-objcopy/MachO/remove-section-error.test
99

it's kinda hard to remove things from here because it's tedious and error-prone to manually recalculate (e.g. harder than for ELF) the offsets specified in various places (e.g. in the load commands) and still get a valid binary (so that it can be parsed by llvm-readobj/llvm-objdump/otool).

this is particular test is based on a real world binary:

a.c: 
    __attribute__((section("__DATA,C"))) int a = 2;
    int f() { return a; }
clang -c a.c -o a.o
llvm/tools/llvm-objcopy/MachO/Object.cpp
1

sure, will do

alexshap marked an inline comment as done.Apr 20 2020, 1:38 AM
alexshap added inline comments.
llvm/test/tools/llvm-objcopy/MachO/remove-section-error.test
99

typo: this particular test is ...

alexshap marked an inline comment as done.Apr 20 2020, 2:01 AM
alexshap added inline comments.
llvm/test/tools/llvm-objcopy/MachO/remove-section-error.test
99

clang -fno-exceptions -fno-unwind-tables -c a.o seems to work (to get rid of eh_frame, __compact_unwind), I'll trim down the test tomorrow.

jhenderson added inline comments.Apr 20 2020, 5:24 AM
llvm/test/tools/llvm-objcopy/MachO/remove-section-dead-symbols.test
5

I guess my question is more along the lines of "we test the section is removed elsewhere, so do we need to test here?" Happy either way, if you think there's additional value.

llvm/test/tools/llvm-objcopy/MachO/remove-section-error.test
99

Okay, got it. It might be worth including the input and compilation command somewhere in a comment, so that the output can be recreated if needed.

alexshap marked an inline comment as done.Apr 20 2020, 5:07 PM
alexshap added inline comments.
llvm/tools/llvm-objcopy/MachO/Object.cpp
31

this could be "remove_if" if "remove_if" didn't modify the elements.
https://en.cppreference.com/w/cpp/algorithm/remove
https://en.cppreference.com/w/cpp/algorithm/stable_partition
https://iterator.wordpress.com/2016/01/31/algorithms_0/ .
Quite the opposite - with std::stable_partition we preserve the order of sections which have not been removed.

The Mach-O bits look good to me.

llvm/tools/llvm-objcopy/MachO/Object.cpp
31

Right, makes sense.

alexshap updated this revision to Diff 258879.Apr 20 2020, 5:59 PM

Address comments

alexshap marked an inline comment as done.Apr 20 2020, 6:01 PM
jhenderson accepted this revision.Apr 21 2020, 12:32 AM

LGTM, with one outstanding comment/question.

llvm/test/tools/llvm-objcopy/MachO/remove-section-dead-symbols.test
5

I guess my question is more along the lines of "we test the section is removed elsewhere, so do we need to test here?" Happy either way, if you think there's additional value.

Just flagging this repsonse up in case you missed it. Do you think there's additional value in testing it here as well as in the regular --remove-section test?

This revision is now accepted and ready to land.Apr 21 2020, 12:32 AM
alexshap updated this revision to Diff 258928.Apr 21 2020, 1:40 AM

Update test

alexshap marked an inline comment as done.Apr 21 2020, 1:42 AM
alexshap added inline comments.
llvm/test/tools/llvm-objcopy/MachO/remove-section-dead-symbols.test
5

I don't have any strong preferences regarding this, I've updated the test (removed those lines).

jhenderson accepted this revision.Apr 21 2020, 2:08 AM
seiya accepted this revision.Apr 21 2020, 5:42 PM
seiya added a comment.EditedApr 21 2020, 5:44 PM

LGTM once clang-format warnings are addressed and build bots are happy.

alexshap updated this revision to Diff 259140.Apr 21 2020, 6:01 PM

Apply clang-format

MaskRay added inline comments.Apr 21 2020, 6:12 PM
llvm/test/tools/llvm-objcopy/MachO/remove-section-dead-symbols.test
5

Delete --sections since the section lines are dropped.

MaskRay added inline comments.Apr 21 2020, 6:14 PM
llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
117

Why is the type change?

alexshap updated this revision to Diff 259148.Apr 21 2020, 7:07 PM
alexshap marked an inline comment as done.Apr 21 2020, 7:10 PM
alexshap marked an inline comment as done.
alexshap added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
117

because in Section Index has this type.

alexshap updated this revision to Diff 259153.Apr 21 2020, 7:47 PM

(fix formatting)

MaskRay accepted this revision.Apr 21 2020, 7:59 PM
This revision was automatically updated to reflect the committed changes.