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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
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. |
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.)
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.
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.
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.
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.
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.