This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by alexander-shaposhnikov 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

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.

alexander-shaposhnikov marked an inline comment as done.Apr 20 2020, 1:28 AM
alexander-shaposhnikov 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"]

alexander-shaposhnikov marked 2 inline comments as done.Apr 20 2020, 1:36 AM
alexander-shaposhnikov 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

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

typo: this particular test is ...

alexander-shaposhnikov marked an inline comment as done.Apr 20 2020, 2:01 AM
alexander-shaposhnikov 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.

alexander-shaposhnikov marked an inline comment as done.Apr 20 2020, 5:07 PM
alexander-shaposhnikov 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.

alexander-shaposhnikov 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
alexander-shaposhnikov marked an inline comment as done.Apr 21 2020, 1:42 AM
alexander-shaposhnikov 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.

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
114

Why is the type change?

alexander-shaposhnikov marked an inline comment as done.Apr 21 2020, 7:10 PM
alexander-shaposhnikov marked an inline comment as done.
alexander-shaposhnikov added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
114

because in Section Index has this type.

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