This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML][MachO] Add basic chained fixups support
AbandonedPublic

Authored by keith on Feb 13 2022, 10:04 AM.

Details

Summary

This adds the most basic versions of chained fixups / dyld exports trie
support by encoding and decoding the bytes directly instead of with a
structured approach. This is at least useful to be able to write tests
from other tools without having to check in binaries, but in the future
we might want to replace this format with one mirroring the actual
format.

Diff Detail

Event Timeline

keith created this revision.Feb 13 2022, 10:04 AM
keith requested review of this revision.Feb 13 2022, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2022, 10:04 AM

I don't really know Mach-O myself, so I can't really comment on the correctness of this patch.

llvm/lib/Object/MachOObjectFile.cpp
1384–1387

I see this pattern is very common. I think it would be worth factoring out the repetivtive function call into a lambda, to save repeating most of the arguments (*this, Load, I, and Elements appear to be common to all checkLinkeditDataCommand). This might be worth doing as a prerequisite patch, and tidying up the checkDyldInfoCommand calls in the same manner. There might be other refactorings worth doing to reduce repetitiveness even further, but that's probably going a bit far.

llvm/test/ObjectYAML/MachO/chained_fixups.yaml
184–185

These look incomplete to me?

keith updated this revision to Diff 408670.Feb 14 2022, 4:56 PM
keith marked an inline comment as done.

Include all data in test

llvm/lib/Object/MachOObjectFile.cpp
1384–1387

Yea I can think about that as a follow up, it's a bit of a pain with how these string + constant representations are duplicated

llvm/test/ObjectYAML/MachO/chained_fixups.yaml
184–185

Yea I was intentionally adding just enough to make sure something came through here, but I can verify 1:1 to be safe

Okay, I'm happy with the comments I made, but as mentioned, someone else will need to look at this patch from a Mach-O perspective.

llvm/test/ObjectYAML/MachO/chained_fixups.yaml
185

CHECK-NEXT here and below.

keith updated this revision to Diff 408960.Feb 15 2022, 10:29 AM
keith marked an inline comment as done.

Use CHECK-NEXT for order of bytes

MaskRay accepted this revision.Apr 30 2022, 12:54 PM
MaskRay added a subscriber: MaskRay.

LGTM

llvm/lib/ObjectYAML/MachOEmitter.cpp
522

emplace_back

527

emplace_back

This revision is now accepted and ready to land.Apr 30 2022, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2022, 12:54 PM
Herald added a subscriber: StephenFan. · View Herald Transcript

What's the status on this? The changes look good to me, and have already been approved by @MaskRay.

I would like to use this for a test in D129430.

keith added a comment.Jul 11 2022, 3:28 PM

I paused here as on another diff @aprantl mentioned they were working on a more sophisticated version of this. @aprantl any update on your side?

thakis added a subscriber: thakis.Jul 13 2022, 5:14 AM

Given we're kind of blocked on this, let's land this and then replace it with the better version once it's done. (That shouldn't cause aprantl more work, since reverting this before landing their change is easy.)

thakis added inline comments.Jul 13 2022, 5:21 AM
llvm/lib/Object/MachOObjectFile.cpp
1387

Hm, is this diff very old? phab says "Feb 2022", but…oh I see, https://reviews.llvm.org/D113630 is aprantl's change that landed concurrently with this (?)

I think we can do something like in llvm/test/Object/Inputs/MachO/chained-fixups.yaml for our thing, so we're (probably) not blocked on this after all.

But it looks to me like aprantl's thing is maybe landed and this just needs rebasing.

keith added a comment.Jul 20 2022, 9:43 AM

I think we can do something like in llvm/test/Object/Inputs/MachO/chained-fixups.yaml for our thing, so we're (probably) not blocked on this after all.

But it looks to me like aprantl's thing is maybe landed and this just needs rebasing.

Given the workaround of just including the raw section info, I wonder now if this is worth doing unless we actually support more sophisticated structure, what do you think?

Adding @Alpha as a reviewer, who expressed interest in continuing my effort to upstream the Apple implementation.

thakis added a comment.Aug 8 2022, 4:14 AM

Adding @Alpha as a reviewer, who expressed interest in continuing my effort to upstream the Apple implementation.

We're interested in adding support for chained fixups to ld64.lld soon. So it'd be nice if you could upstream things soon; else we'll have to reimplement them until the upstreaming changes arrive.

keith abandoned this revision.Aug 15 2022, 10:46 AM

Dropping this for the upstream apple stuff hopefully coming in the future. We can reopen if needed. For now you can just save linkedit data directly.