Page MenuHomePhabricator

[lld-macho] Fix alignment & layout to match ld64 and satisfy codesign
ClosedPublic

Authored by gkm on Jan 18 2021, 3:06 PM.

Details

Summary

Dyld on arm64 macOS has strict requirements for alignment and sequence of segments and sections.

I developed this diff by incrementally changing alignments & sequences to match the output of ld64. I stopped when arm64 macOS began executing my test programs rather than immediately rejecting them in execve(2) with errno == EBADMACHO "Malformed Mach-O file".

Diff Detail

Event Timeline

gkm created this revision.Jan 18 2021, 3:06 PM
gkm requested review of this revision.Jan 18 2021, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2021, 3:06 PM

Could you please add a test case for verifying the output order of the load commands? It seems that the load commands are expected to be in a certain order according to your change - it makes sense to have a test which validates that ordering.

lld/MachO/SyntheticSections.cpp
394

Hmm, this is 4 and not WordSize? It is plausible, but might be reasonable to add a comment explaining why 4 on 64-bit systems is needed/correct.

420

Similar.

lld/MachO/Target.h
30 ↗(On Diff #317425)

Isn't the page size 4K on x64 and 16K on ARM64?

lld/MachO/UnwindInfoSection.cpp
90

IIRC, 4 is correct for the __unwind_info section, but this probably deserves a comment (unless Im mixing up the __compact_unwind_entries).

gkm updated this revision to Diff 321292.Feb 3 2021, 7:00 PM
gkm marked 4 inline comments as done.
  • remove PageSize change, which is premature.
gkm added a comment.Feb 3 2021, 7:51 PM

@compnerd: I am uncertain how to structure a good test. A good test for satisfactory load-command order is already implicit: lld-linked programs don't run on ARM64 macOS otherwise.

lld/MachO/SyntheticSections.cpp
394

__stubs and __stub_helper are text sections populated with machine instruction, so 4-byte alignment is appropriate.

lld/MachO/Target.h
30 ↗(On Diff #317425)

This belongs in the ARM64 target diff.

lld/MachO/UnwindInfoSection.cpp
90

In all cases, I choose alignment boundaries to mimic ld64. Do you want comment(s) to say that?

int3 added a subscriber: int3.Feb 3 2021, 9:39 PM

I am uncertain how to structure a good test.

The output of llvm-objdump --macho --all-headers should allow you to verify the ordering of both load commands and segment/sections.

lld/MachO/SyntheticSections.cpp
394

Maybe naive question: So this means arm64/x86_64 instructions are only 4-byte-aligned?

In any case, this still needs a comment :)

lld/MachO/UnwindInfoSection.cpp
90

Yeah it's worth mentioning IMO to explain why we are not using a larger value. Is this also the minimum alignment, i.e. do we get EBADMACHO if a smaller alignment is used? That's worth mentioning too...

Relatedly, I'm a bit confused by the commit title/message. The commit title mentions codesign, the message mentions dyld first, but later talks about execve and EBADMACHO, which leads me to believe that the error is coming from the kernel. (At least, it looks like EBADMACHO is generated here: https://github.com/apple/darwin-xnu/blob/8f02f2a044b9bb1ad951987ef5bab20ec9486310/bsd/kern/kern_exec.c#L6478).

lld/test/MachO/symtab.s
13

FileCheck does substring matches by default, so we can just skip checking the value at the end

(ditto for the lines below)

gkm marked 2 inline comments as done.Fri, Feb 5, 10:35 AM
gkm added inline comments.
lld/MachO/SyntheticSections.cpp
394

Yes. arm64 instructions are fixed-width 4 bytes, with natural 4-byte alignment. x86_64 instructions are variable width and have no alignment requirement, but ld64 aligns them to 4 bytes anyway.

lld/MachO/UnwindInfoSection.cpp
90

Regarding whether we get EBADMACHO at smaller alignment: unknown. I corrected alignments to mimic ld64 until codesign & execve(2) began working and never experimented with underaligning anything to see if it still worked. That never seemed a worthy use of time.

I clarified the commit summary: codesign & kernel execve(2) failures are what guided my changes. dyld(1) is likely also picky, but never gets a chance to run until codesign and kernel are happy.

gkm updated this revision to Diff 321827.EditedFri, Feb 5, 10:51 AM
gkm marked 2 inline comments as done.
  • revise according to review feedback
  • add test
int3 accepted this revision.Fri, Feb 5, 2:17 PM
int3 added inline comments.
lld/test/MachO/load-command-sequence.s
19
This revision is now accepted and ready to land.Fri, Feb 5, 2:17 PM
gkm marked an inline comment as done.Fri, Feb 5, 4:21 PM
gkm updated this revision to Diff 321895.Fri, Feb 5, 4:22 PM
  • nit + rebase
This revision was landed with ongoing or failed builds.Fri, Feb 5, 4:23 PM
This revision was automatically updated to reflect the committed changes.