refactor(macho): abstract bind/rebase resolution behind protocol#82
refactor(macho): abstract bind/rebase resolution behind protocol#82
Conversation
Introduce MachOBindRebaseResolving so indirect symbolic reference readers no longer hard-cast to MachOFile. Wrapper types (UI-layer projections that compose a MachOFile, etc.) can opt in by conforming and forwarding to their underlying MachOFile. - Add MachOBindRebaseResolving in MachOExtensions; MachOFile conforms - Expose bindRebaseResolver on ReadingContext (default nil) and on MachOContext (cast-through when the underlying MachO conforms) - SymbolOrElementPointer.resolve consults the protocol via context lookup instead of casting to MachOFile / MachOContext<MachOFile> - Add generic contextDescriptor(in:) overload on TypeMetadataRecord
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the MachOBindRebaseResolving protocol to abstract dyld bind and rebase resolution, allowing ReadingContext and SymbolOrElementPointer to handle relocations generically without hardcoding MachOFile. It also adds contextDescriptor resolution to TypeMetadataRecord. A review comment suggests removing a redundant do-catch block in MachOFile+Swift.swift that uses print(), as it pollutes standard output and the error is already re-thrown.
| return try records.compactMap { | ||
| do { | ||
| return try $0.contextDescriptor(in: machO) | ||
| } catch { | ||
| print(error) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid using print() in library code as it pollutes the standard output of the consuming application. Since the error is already being re-thrown and the caller is responsible for handling it, this do-catch block is redundant and should be removed to keep the code clean.
return try records.compactMap { try $0.contextDescriptor(in: machO) }There was a problem hiding this comment.
Pull request overview
This PR refactors bind/rebase (dyld chained fixups) resolution so indirect symbolic-reference readers can consult an abstract resolver rather than hard-casting to MachOFile, enabling wrapper types to participate by forwarding.
Changes:
- Add
MachOBindRebaseResolvingprotocol (withMachOFileconformance) and expose it viaReadingContext.bindRebaseResolver/MachOContext. - Update
SymbolOrElementPointer.resolveto use the resolver from the MachO object or reading context instead of hard-casting. - Add a
ReadingContext-basedcontextDescriptor(in:)overload onTypeMetadataRecordto support the new context-driven resolution path.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/MachOSymbolPointers/SymbolOrElementPointer.swift | Switches bind/rebase lookup to a protocol/context-based resolver. |
| Sources/MachOSwiftSection/Models/Type/TypeMetadataRecord.swift | Adds ReadingContext overload to resolve context descriptors via context addresses. |
| Sources/MachOSwiftSection/MachOFile+Swift.swift | Changes type-metadata record mapping to include a print+rethrow debug path. |
| Sources/MachOReading/ReadingContext/ReadingContext.swift | Adds optional bindRebaseResolver to the context API (default nil). |
| Sources/MachOReading/ReadingContext/MachOContext.swift | Provides bindRebaseResolver via runtime cast of the wrapped MachO. |
| Sources/MachOExtensions/MachOBindRebaseResolving.swift | Introduces resolver protocol and makes MachOFile conform. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return try records.compactMap { | ||
| do { | ||
| return try $0.contextDescriptor(in: machO) | ||
| } catch { | ||
| print(error) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
The added do/catch only prints and then rethrows, which is behaviorally redundant but introduces unconditional stdout noise in a library API. Consider removing the do/catch entirely, or route diagnostics through the project’s logging mechanism / a caller-provided logger if you need this visibility.
| return try records.compactMap { | |
| do { | |
| return try $0.contextDescriptor(in: machO) | |
| } catch { | |
| print(error) | |
| throw error | |
| } | |
| } | |
| return try records.compactMap { try $0.contextDescriptor(in: machO) } |
| func resolveRebase(fileOffset: Int) -> UInt64? | ||
| } | ||
|
|
||
| extension MachOFile: MachOBindRebaseResolving {} |
There was a problem hiding this comment.
This is a retroactive conformance of an external type (MachOFile from MachOKit) to a local protocol. The codebase already uses @retroactive for similar conformances; adding it here will avoid Swift’s retroactive-conformance warnings and makes the intent explicit.
| public static func resolve<MachO: MachORepresentableWithCache & Readable>(from offset: Int, in machO: MachO) throws -> Self { | ||
| if let machOFile = machO as? MachOFile { | ||
| if let symbol = machOFile.resolveBind(fileOffset: offset) { | ||
| if let resolver = machO as? any MachOBindRebaseResolving { | ||
| if let symbol = resolver.resolveBind(fileOffset: offset) { | ||
| return .symbol(.init(offset: offset, name: symbol)) | ||
| } else { | ||
| let resolvedFileOffset = offset | ||
| if let rebase = machOFile.resolveRebase(fileOffset: resolvedFileOffset) { | ||
| return .address(rebase) | ||
| } else { | ||
| return try .address(machOFile.readElement(offset: resolvedFileOffset)) | ||
| } | ||
| } | ||
| } else { | ||
| return try .address(machO.readElement(offset: offset)) | ||
| if let rebase = resolver.resolveRebase(fileOffset: offset) { | ||
| return .address(rebase) | ||
| } | ||
| } | ||
| return try .address(machO.readElement(offset: offset)) |
There was a problem hiding this comment.
PR description says indirect symbolic-reference readers “stop hard-casting to MachOFile”, but there are still hard casts elsewhere (e.g. Sources/MachOSymbols/SymbolOrElement.swift). Either expand this refactor to cover those remaining call sites, or adjust the PR description to reflect the narrower scope.
The debug `print(error)` in `_readTypeMetadataRecords` was a leftover from local debugging and not part of the bind/rebase abstraction.
Summary
MachOBindRebaseResolvingso indirect symbolic-reference readers stop hard-casting toMachOFile, letting wrapper types (e.g. UI-layer projections that compose aMachOFile) opt in by forwarding to the underlying file.bindRebaseResolveronReadingContext(defaultnil) and onMachOContext(runtime cast so the genericMachOparameter stays unconstrained), and makeSymbolOrElementPointer.resolveconsult the protocol via context lookup instead of casting.contextDescriptor(in:)overload onTypeMetadataRecordto align with the new context-driven resolution path.Test plan
swift package update && swift buildswift testswift run swift-section dump <path>to confirm indirect symbolic references still resolve through bind/rebase.