Bug fixes and some optimizations #17
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Mx-Iris, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant architectural overhaul and refactoring across the MachO parsing and Swift demangling components. The primary goal is to enhance the generality, robustness, and maintainability of the codebase by abstracting core operations, introducing a unified caching system, and improving the accuracy and flexibility of Swift symbol demangling and data dumping.
Highlights
- Core Abstraction Refactoring: The MachO reading, symbol resolution, and pointer handling logic have been significantly refactored to operate on a new, more generic
MachORepresentableWithCache & MachOReadableprotocol. This enhances flexibility and reusability across different MachO contexts (files, images, caches) by moving away from concreteMachOFileorMachOImagetypes. - New Caching Infrastructure: A new
MachOCacheclass has been introduced as a generic base for caching various data, such as symbols and primitive type mappings. This new caching mechanism includes memory pressure monitoring to automatically clear caches on memory warnings or critical events, optimizing resource usage.SymbolCacheandPrimitiveTypeMappingCachenow leverage this unified caching system. - Enhanced Swift Demangling: The Swift demangling process has been improved with the addition of a new
invalidSwiftMangledNameerror, aremoveBoundGenericoption for cleaner output, and the removal of outdated Swift 3 demangling support. The core demangling logic has been streamlined by movinggetManglingPrefixLengthinto theDemanglerstruct and removing extensive Swift 3-specific code. - Unified Symbol and Element Resolution: A new
MachOResolvingmodule andResolvableprotocol, along with theSymbolOrElementPointerenum, provide a robust and unified way to resolve symbols and elements. This new abstraction effectively handles both direct memory addresses and dynamically bound/rebased symbols, simplifying pointer and data resolution throughout the codebase. - Swift Section Dumping Overhaul: The entire Swift section dumping functionality has been rewritten to align with the new generalized MachO types. A structured
Dumpableprotocol hierarchy (includingConformedDumpable,NamedDumpable, and their respectiveDumperprotocols) has been introduced, making the output formatting more organized and extensible. - Macro Usage Reduction: The
@MachOImageGeneratorand@MachOImageAllMembersGeneratormacros have been commented out, indicating a shift from macro-generated code to more explicit, manually implemented functionalities. This change affects numerous files across theMachOExtensions,MachOPointer,MachOSwiftSection, andMachOSymbolsmodules.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the codebase. The core change is the introduction of a generic abstraction layer (MachORepresentableWithCache, Resolvable, etc.) to unify logic for file-based (MachOFile) and memory-based (MachOImage) Mach-O representations. This greatly improves the architecture, reduces code duplication, and enhances modularity.
Key improvements include:
- A new caching infrastructure (
MachOCache) to improve performance. - Removal of deprecated Swift 3 demangling logic, which likely fixes bugs with misinterpreting symbols.
- A cleaner API for tree traversal in the
Demanglemodule. - A large number of
packageAPIs have been madepublic, expanding the library's usability.
The changes are extensive but consistent. I've identified a few areas with redundant code (unnecessary type casts and a redundant variable) that can be cleaned up for better clarity. Overall, this is a high-quality pull request that significantly improves the project's design.
| let resolvedFileOffset = offset | ||
| if let rebase = machOFile.resolveRebase(fileOffset: resolvedFileOffset) { | ||
| return .address(rebase) | ||
| } else { | ||
| return try .address(machOFile.readElement(offset: resolvedFileOffset)) | ||
| } |
There was a problem hiding this comment.
The variable resolvedFileOffset is redundant as it's just a copy of offset. You can use offset directly in the subsequent calls to improve clarity and remove unnecessary code.
if let rebase = machOFile.resolveRebase(fileOffset: offset) {
return .address(rebase)
} else {
return try .address(machOFile.readElement(offset: offset))
}|
|
||
| if descriptor.flags.numConditionalRequirements > 0 { | ||
| self.conditionalRequirements = try machOFile.readElements(offset: currentOffset, numberOfElements: descriptor.flags.numConditionalRequirements.cast()) | ||
| self.conditionalRequirements = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.flags.numConditionalRequirements.cast()) as [GenericRequirementDescriptor] |
There was a problem hiding this comment.
The type cast as [GenericRequirementDescriptor] is redundant. The compiler can infer the generic parameter Element for readWrapperElements from the type of the conditionalRequirements property. Removing the explicit cast makes the code cleaner. A similar issue exists on line 87 for resilientWitnesses.
| self.conditionalRequirements = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.flags.numConditionalRequirements.cast()) as [GenericRequirementDescriptor] | |
| self.conditionalRequirements = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.flags.numConditionalRequirements.cast()) |
| self.resilientWitnessesHeader = header | ||
| currentOffset.offset(of: ResilientWitnessesHeader.self) | ||
| self.resilientWitnesses = try machOFile.readElements(offset: currentOffset, numberOfElements: header.numWitnesses.cast()) | ||
| self.resilientWitnesses = try machO.readWrapperElements(offset: currentOffset, numberOfElements: header.numWitnesses.cast()) as [ResilientWitness] |
There was a problem hiding this comment.
The type cast as [ResilientWitness] is redundant. The compiler can infer the generic parameter Element for readWrapperElements from the type of the resilientWitnesses property. Removing the explicit cast makes the code cleaner.
| self.resilientWitnesses = try machO.readWrapperElements(offset: currentOffset, numberOfElements: header.numWitnesses.cast()) as [ResilientWitness] | |
| self.resilientWitnesses = try machO.readWrapperElements(offset: currentOffset, numberOfElements: header.numWitnesses.cast()) |
|
|
||
| if descriptor.numRequirementsInSignature > 0 { | ||
| self.requirementInSignatures = try machOFile.readElements(offset: currentOffset, numberOfElements: descriptor.numRequirementsInSignature.cast()) | ||
| self.requirementInSignatures = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.numRequirementsInSignature.cast()) as [GenericRequirementDescriptor] |
There was a problem hiding this comment.
The type cast as [GenericRequirementDescriptor] is redundant. The compiler can infer the generic parameter Element for readWrapperElements from the type of the requirementInSignatures property. Removing the explicit cast makes the code cleaner. A similar issue exists on line 53 for requirements.
| self.requirementInSignatures = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.numRequirementsInSignature.cast()) as [GenericRequirementDescriptor] | |
| self.requirementInSignatures = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.numRequirementsInSignature.cast()) |
|
|
||
| if descriptor.numRequirements > 0 { | ||
| self.requirements = try machOFile.readElements(offset: currentOffset, numberOfElements: descriptor.numRequirements.cast()) | ||
| self.requirements = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.numRequirements.cast()) as [ProtocolRequirement] |
There was a problem hiding this comment.
The type cast as [ProtocolRequirement] is redundant. The compiler can infer the generic parameter Element for readWrapperElements from the type of the requirements property. Removing the explicit cast makes the code cleaner.
| self.requirements = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.numRequirements.cast()) as [ProtocolRequirement] | |
| self.requirements = try machO.readWrapperElements(offset: currentOffset, numberOfElements: descriptor.numRequirements.cast()) |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the MachO dumping infrastructure to use generic MachO abstractions, centralized caching, and a new Dumpable protocol hierarchy, while updating tests to the new APIs.
- Adopt
MachOCachefor symbol, index, and primitive-type caching and replaceMachOFile–specific APIs with genericMachORepresentableWithCache & MachOReadablemethods - Introduce the
Dumpable,NamedDumpable, andConformedDumpableprotocols and move each type’s dump logic into dedicated Dumper structs - Remove legacy
Dumpable+extensions, update commands, and adjust tests to the generic signatures
Reviewed Changes
Copilot reviewed 126 out of 127 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/SwiftDumpTests/DyldCacheDumpTests.swift | Remove image-name override—update required after generics refactor |
| Tests/SwiftDumpTests/XcodeMachOFileDumpTests.swift | Add explicit fileName override for dump tests |
| Sources/swift-section/DemangleCommand.swift | Handle optional demangling and update print fallback |
| Sources/SwiftDump/PrimitiveTypeMappingCache.swift | New cache class for primitive type mappings |
| Sources/SwiftDump/Dumpable/Struct+Dumpable.swift | Move struct dumping into StructDumper |
| Sources/SwiftDump/Extensions/String+.swift | Add insertSubFunctionPrefix for fallback symbol naming |
| Sources/MachOSymbols/SymbolCache.swift | Migrate to MachOCache, add lazy demangle caching |
| Sources/MachOSymbols/SymbolIndexStore.swift | Migrate to MachOCache, remove manual memory-pressure logic |
| Sources/MachOSwiftSection/Utils/MetadataReader.swift | Generalize demangle/read APIs to generic MachO types |
| Sources/MachOSwiftSection/Protocols/MachOSwiftSectionRepresentableWithCache.swift | Make protocol public for generic constraints |
Comments suppressed due to low confidence (3)
Sources/SwiftDump/Extensions/String+.swift:35
- [nitpick] The method
insertSubFunctionPrefixcould be renamed to something more descriptive (e.g.withSubFunctionPrefix(_:)orprefixedWithSubFunction()) to clarify its intent.
var insertSubFunctionPrefix: String {
Tests/SwiftDumpTests/DyldCacheDumpTests.swift:11
- The override for
cacheImageNamewas removed, so these tests will now use the base class default instead of.SwiftUI. Reintroduceoverride class var cacheImageName: MachOImageName { .SwiftUI }to maintain the original target image.
final class DyldCacheDumpTests: DyldCacheTests, DumpableTests {}
Sources/MachOSwiftSection/Utils/MetadataReader.swift:20
- This method now returns an optional
Node?, but many call sites assume non-optional. Verify callers handlenilproperly or make the return non-optional if a symbol should always demangle.
public static func demangleSymbol<MachO: MachORepresentableWithCache & MachOReadable>(for unsolvedSymbol: Symbol, in machO: MachO) throws -> Node? {
| // for symbol in cacheEntry.symbolsByOffset.values.flatMap({ $0 }) { | ||
| // do { | ||
| // let node = try demangleAsNode(symbol.stringValue) | ||
| // cacheEntry.demangledNodeBySymbol[symbol] = node | ||
| // } catch { | ||
| // print(error) | ||
| // } |
There was a problem hiding this comment.
[nitpick] There are large commented-out blocks for symbol prefetching which can be removed to reduce noise and prevent confusion.
| // for symbol in cacheEntry.symbolsByOffset.values.flatMap({ $0 }) { | |
| // do { | |
| // let node = try demangleAsNode(symbol.stringValue) | |
| // cacheEntry.demangledNodeBySymbol[symbol] = node | |
| // } catch { | |
| // print(error) | |
| // } | |
| // (Commented-out block removed) |
| extension ResilientSuperclass { | ||
| @MachOImageGenerator | ||
| package func dumpSuperclass(using options: DemangleOptions, for kind: TypeReferenceKind, in machOFile: MachOFile) throws -> SemanticString? { | ||
| package func dumpSuperclass<MachO: MachOSwiftSectionRepresentableWithCache & MachOReadable>(using options: DemangleOptions, for kind: TypeReferenceKind, in machO: MachO) throws -> SemanticString? { |
There was a problem hiding this comment.
[nitpick] This signature is long and repetitive; consider introducing a typealias for the generic constraint to reduce boilerplate and improve readability.
No description provided.