A much faster implementation for accessing the dynamic linker and other info necessary for crash reports and symbolication #683
Conversation
c778df7 to
41f0404
Compare
…er info necessary for crash reports and symbolication
1a64537 to
3cce42a
Compare
3cce42a to
eacb0c8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the existing binary image caching mechanism with a significantly faster dynamic linker implementation. The changes eliminate the old KSBinaryImageCache module and introduce a new lazy-loading approach in KSDynamicLinker that pre-processes all relevant Mach-O data structures during a single pass.
Key changes include:
- Complete rewrite of the dynamic linker cache with lazy-loading and pre-computation of offsets for 5-10x performance improvement
- Separation of crash info and symbolication into dedicated result structures instead of embedding them in image data
- Migration from
ksbic_*functions toksdl_*functions across all consuming code
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| KSDynamicLinker.h | Redesigned API with new types for binary images, crash info, and symbolication results |
| KSDynamicLinker.c | Complete rewrite implementing lazy-loading cache with pre-computed segment and symbol table pointers |
| KSBinaryImageCache.h/.c | Removed old caching implementation |
| KSDynamicLinker_Tests.m | Updated tests to use new API with comprehensive validation of cached data |
| KSBinaryImageCache_Tests.m | Removed test file for deleted module |
| KSBacktrace.c | Updated to use new symbolication API |
| KSSymbolicator.c | Simplified to use new symbolication function |
| KSCrashReportC.c | Updated to use new image enumeration and crash info APIs |
| KSCrashC.c | Updated initialization to use new dynamic linker |
| KSCrashMonitor*.c/.m | Updated to use new APIs and added debug logging |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // This should never happen, but if it does we need to contain the damage. | ||
| // Fallout: Some images won't be available for symbolication. | ||
| if (imageCount > g_state.imagesCapacity) { | ||
| KSLOG_ERROR("Images count %d > than capacity %d", imageCount, g_state.imagesCapacity); |
There was a problem hiding this comment.
The error message contains a grammatical error. It should be "Images count %d > capacity %d" or "Images count %d greater than capacity %d".
| KSLOG_ERROR("Images count %d > than capacity %d", imageCount, g_state.imagesCapacity); | |
| KSLOG_ERROR("Images count %d > capacity %d", imageCount, g_state.imagesCapacity); |
Copilot uses AI. Check for mistakes.
| // This should never happen, but if it does we need to contain the damage. | ||
| // Fallout: Some images won't be available for symbolication. | ||
| if (imageCount > g_state.imagesCapacity) { | ||
| KSLOG_ERROR("Images count %d > than capacity %d", imageCount, g_state.imagesCapacity); |
There was a problem hiding this comment.
The format specifier %d is used for size_t variables imageCount and g_state.imagesCapacity. This should be %zu for size_t to avoid potential issues on different architectures.
| KSLOG_ERROR("Images count %d > than capacity %d", imageCount, g_state.imagesCapacity); | |
| KSLOG_ERROR("Images count %zu > than capacity %zu", imageCount, g_state.imagesCapacity); |
Copilot uses AI. Check for mistakes.
| size_t actualCount = _dyld_image_count(); | ||
|
|
||
| XCTAssertGreaterThan(cachedCount, 0, @"There should be at least some images loaded"); | ||
| XCTAssertGreaterThanOrEqual(cachedCount, actualCount, @"Cached count should be at least 100%% of actual count"); |
There was a problem hiding this comment.
The assertion message contains unnecessary double percent signs. It should be "Cached count should be at least 100% of actual count" instead of "100%% of actual count".
| XCTAssertGreaterThanOrEqual(cachedCount, actualCount, @"Cached count should be at least 100%% of actual count"); | |
| XCTAssertGreaterThanOrEqual(cachedCount, actualCount, @"Cached count should be at least 100% of actual count"); |
Copilot uses AI. Check for mistakes.
You mean release 3.0? |
|
If we keep on 2.x, then the next .x bump. I just want to avoid too many changes to the fundamentals of the reporter in a single release (in case god forbid something goes wrong). Binary image handling and symbolication is tricky business, and I'd like to give more time to shake any bugs out before it goes mainstream. |
Note: Not to be merged until AFTER the next release. We've had enough significant changes already for one release!
New, faster, easier dynamic linker interface (
dladdrwas such an awful API).Instead of repeatedly walking the many lists in the MACH-O images for every single lookup, we do only one pass and gather all relevant pointers and data so as to greatly speed up future lookups (including symbolication).
Crash info is no longer part of the image data, but is instead fetched separately, and returned as a mini struct (we're only going to access crash info data once per crash, so it makes more sense to separate them). Symbolication also returns a mini-struct with the results.
I've achieved speedups ranging from 5x to 10x (situationally), so the info gathering (and especially symbolication process) should be a lot quicker now. At this point, the JSON encoder is the biggest drag, but I have a future plan for that as well!
Images are lazy-loaded to avoid a high upfront cost (this is especially important since the initial refresh will happen during app launch). Only the address and file paths of each image is fetched at first (from the dynamic linker's image list). Any other information is only available after lazy-loading.
KSDynamicLinker keeps a mirror of the dynamic linker's images list, keeping the same headers in the same positions so as to make updates quick. This can result in unnecessary cache invalidations, but I think the tradeoff in simplicity is worth it. I've added many comments in
KSDynamicLinker.cand the header, which hopefully explain well enough how it operates. It'll probably be easier to just look at the finishedKSDynamicLinker.crather than the diff since almost all of the old code is gone now.