Fixes for RelativeListList separated into different cache files #14
Conversation
- Simplified offset resolution logic - Removed unnecessary variable assignments - Enhanced readability of file handle and offset retrieval
- Introduced two locateValue methods for locating values in DyldCache - Supports both KeyPath and custom resolver functions - Enhances functionality for handling subCaches
- Rename typealias from LocateValue to LocatedValue - Update documentation for locateValue methods - Ensure consistent return types across locateValue implementations
- Introduced new properties for ObjC optimizations and header optimizations - Updated machO retrieval methods to use new properties - Removed redundant methods from ObjCHeaderInfoROProtocol and RelativeListListEntry
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues with RelativeListList handling when data is separated across different dyld cache files. The changes introduce a systematic approach to locating values across cache hierarchies and streamline how MachO files are retrieved from cache indices.
- Adds a generic
locateValuemechanism to search across cache hierarchies (main cache and subcaches) - Replaces direct extension methods on
RelativeListListEntrywith cache-based lookups - Introduces
_machOhelper returning located values to properly handle cross-cache references
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/MachOObjCSection/Support/ObjCDump.swift | Adds workaround extension for MachOFile.imagePath |
| Sources/MachOObjCSection/Model/Util/RelativeListList.swift | Refactors offset resolution and removes RelativeListListEntry extensions |
| Sources/MachOObjCSection/Model/Protocol/ObjCProtocolRelativeListList.swift | Updates to use cache._machO instead of entry.machO |
| Sources/MachOObjCSection/Model/Property/ObjCPropertyRelativeListList.swift | Updates to use cache._machO instead of entry.machO |
| Sources/MachOObjCSection/Model/Method/ObjCMethodRelativeListList.swift | Updates to use cache._machO instead of entry.machO |
| Sources/MachOObjCSection/Extension/ObjCHeaderOptimizationRO+.swift | Removes _machO helper method that's been replaced |
| Sources/MachOObjCSection/Extension/DyldCache+.swift | Major refactoring introducing locateValue mechanism and LocatedValue pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let value = resolver(self) { return (self, value) } | ||
|
|
||
| guard let mainCache else { return nil } |
There was a problem hiding this comment.
[nitpick] Consider using a guard statement pattern to align with the existing code style in this file. Change if let value = resolver(self) { return (self, value) } to guard let value = resolver(self) else { ... } for consistency with lines 74-76 and 77-79.
| if let value = resolver(self) { return (self, value) } | |
| guard let mainCache else { return nil } | |
| guard let value = resolver(self) else { return nil } | |
| return (self, value) |
| } | ||
|
|
||
| extension RelativeListListProtocol { | ||
| public func entries(in machO: MachOFile) -> [Entry] { |
There was a problem hiding this comment.
The refactored code assumes machO.fileHandleAndOffset(forOffset:) exists, but this method is not visible in the diff or context. Add a comment explaining this dependency or ensure the method is documented, especially since this is a critical change from the previous manual offset resolution logic.
| public func entries(in machO: MachOFile) -> [Entry] { | |
| public func entries(in machO: MachOFile) -> [Entry] { | |
| // NOTE: This code depends on `MachOFile.fileHandleAndOffset(forOffset:)` existing. | |
| // This method is expected to resolve a file handle and file offset for a given virtual offset. | |
| // Ensure that `fileHandleAndOffset(forOffset:)` is implemented and documented on `MachOFile`. |
| return nil | ||
| } | ||
| return header._machO(mainCache: mainCache) | ||
| return locateValue { header.machO(in: $0) } |
There was a problem hiding this comment.
[nitpick] The nested locateValue call on line 142 (and 154) could fail to locate the MachO file even when a header is found. Consider adding a comment explaining why this additional search is necessary, or logging when the header exists but the machO cannot be located.
- Restrict access to imagePath to the MachOFile extension only
No description provided.