This is an archive of the discontinued LLVM Phabricator instance.

Implement support for __llvm_addrsig for MachO in llvm-mc
ClosedPublic

Authored by alx32 on Apr 13 2022, 6:37 PM.

Details

Summary

The __llvm_addrsig section is a section that the linker needs for safe icf.
This was not yet implemented for MachO - this is the implementation.
It has been tested with a safe deduplication implementation inside lld.

Diff Detail

Event Timeline

alx32 created this revision.Apr 13 2022, 6:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2022, 6:37 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
alx32 requested review of this revision.Apr 13 2022, 6:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2022, 6:37 PM
smeenai added a subscriber: smeenai.

Adding some more MC reviewers.

You're missing a test case. You should also upload the patch with context (arc does this for you automatically, otherwise use git diff -U99999 if uploading a patch manually).

MaskRay added inline comments.Apr 13 2022, 8:36 PM
llvm/include/llvm/BinaryFormat/MachO.h
178 ↗(On Diff #422707)

The values may be defined by Apple. We cannot randomly grab a value.

tschuett added inline comments.
llvm/include/llvm/BinaryFormat/MachO.h
176 ↗(On Diff #422707)

This one is in my local loader.h. S_ADDRSIG is not there.

D112160 is a similar recent example of adding a custom Mach-O section, and it might be a good reference for adding a test and assigning a section identifier.

alx32 updated this revision to Diff 423450.Apr 18 2022, 12:50 PM

Add unit test & address section ID comments

int3 added inline comments.Apr 20 2022, 12:35 PM
llvm/test/MC/MachO/addrsig-macho.ll
5 ↗(On Diff #423450)

similar to my comment on D123752, let's minimize this test case

117 ↗(On Diff #423450)

would be nice to follow up with a diff that teaches objdump to properly dump this section

alx32 updated this revision to Diff 424049.Apr 20 2022, 3:28 PM
kyulee added a subscriber: kyulee.Apr 21 2022, 2:53 PM
int3 added a comment.Apr 22 2022, 11:07 AM

@MaskRay do you feel comfortable reviewing this?

Placing code gen test/MC

llvm/include/llvm/MC/MCObjectWriter.h
37

Moving the small boolean variable after the vector. If someone adds more smaller variables to the class in the future, it will make the class size smaller.

llvm/lib/MC/MachObjectWriter.cpp
767

*fragmentList.begin() the dereference has implied that it can't be nullptr, so assert can be removed.

773

drop brace

llvm/test/MC/MachO/addrsig-macho.ll
55 ↗(On Diff #424049)

CodeGen/X86/addrsig.ll has some interesting IR objects which may be worth adding here. It seems to me that CodeGen may be a better directory.

ayermolo added inline comments.
llvm/lib/MC/MCMachOStreamer.cpp
34

I think this snuck in?

llvm/lib/MC/MachObjectWriter.cpp
760

replace with actual type

alx32 updated this revision to Diff 425053.Apr 25 2022, 4:05 PM
alx32 marked 2 inline comments as done.
alx32 updated this revision to Diff 425054.Apr 25 2022, 4:12 PM
alx32 marked an inline comment as done.
alx32 marked 4 inline comments as done.
alx32 marked an inline comment as done.
alx32 added a comment.Apr 27 2022, 3:14 PM

@MaskRay, @ayermolo - Addressed all feedback, could you have a look again ? Thanks!

MaskRay accepted this revision.Apr 27 2022, 4:31 PM
MaskRay added inline comments.
llvm/lib/MC/MachObjectWriter.cpp
765

delete the blank line

llvm/test/CodeGen/AArch64/addrsig-macho.ll
6
This revision is now accepted and ready to land.Apr 27 2022, 4:31 PM
alx32 updated this revision to Diff 425648.Apr 27 2022, 4:56 PM
alx32 marked 2 inline comments as done.
alx32 updated this revision to Diff 426846.May 3 2022, 2:51 PM
alx32 updated this revision to Diff 426847.May 3 2022, 2:55 PM
alx32 updated this revision to Diff 426849.
alx32 updated this revision to Diff 426853.May 3 2022, 2:59 PM
This revision was landed with ongoing or failed builds.May 3 2022, 3:19 PM
This revision was automatically updated to reflect the committed changes.