[PLAT-8318] Fix inaccurate isPC & isLR values

Bugsnag/Helpers/BSGKeys.h

@@ -53,11 +53,8 @@ static BSGKey const BSGKeyGroupingHash              = @"groupingHash";
 static BSGKey const BSGKeyHandled                   = @"handled";
 static BSGKey const BSGKeyHandledCount              = @"handledCount";
 static BSGKey const BSGKeyId                        = @"id";
-static BSGKey const BSGKeyImageAddress              = @"image_addr";
-static BSGKey const BSGKeyImageVmAddress            = @"image_vmaddr";
 static BSGKey const BSGKeyIncomplete                = @"incomplete";
 static BSGKey const BSGKeyInfo                      = @"info";
-static BSGKey const BSGKeyInstructionAddress        = @"instruction_addr";
 static BSGKey const BSGKeyIsLaunching               = @"isLaunching";
 static BSGKey const BSGKeyIsLR                      = @"isLR";
 static BSGKey const BSGKeyIsPC                      = @"isPC";
@@ -80,8 +77,6 @@ static BSGKey const BSGKeyMethod                    = @"method";
 static BSGKey const BSGKeyName                      = @"name";
 static BSGKey const BSGKeyNotifier                  = @"notifier";
 static BSGKey const BSGKeyNotifyEndpoint            = @"notify";
-static BSGKey const BSGKeyObjectAddress             = @"object_addr";
-static BSGKey const BSGKeyObjectName                = @"object_name";
 static BSGKey const BSGKeyOrientation               = @"orientation";
 static BSGKey const BSGKeyOsVersion                 = @"osVersion";
 static BSGKey const BSGKeyPayloadVersion            = @"payloadVersion";
@@ -100,8 +95,6 @@ static BSGKey const BSGKeySignal                    = @"signal";
 static BSGKey const BSGKeyStacktrace                = @"stacktrace";
 static BSGKey const BSGKeyStartedAt                 = @"startedAt";
 static BSGKey const BSGKeySymbolAddr                = @"symbolAddress";
-static BSGKey const BSGKeySymbolAddress             = @"symbol_addr";
-static BSGKey const BSGKeySymbolName                = @"symbol_name";
 static BSGKey const BSGKeySystem                    = @"system";
 static BSGKey const BSGKeyThermalState              = @"thermalState";
 static BSGKey const BSGKeyThreads                   = @"threads";

Bugsnag/Payload/BugsnagEvent.m

@@ -314,7 +314,7 @@ - (instancetype)initWithKSCrashReport:(NSDictionary *)event {
     // generate threads/error info
     NSArray *binaryImages = event[@"binary_images"];
     NSArray *threadDict = [event valueForKeyPath:@"crash.threads"];
-    NSArray<BugsnagThread *> *threads = [BugsnagThread threadsFromArray:threadDict binaryImages:binaryImages depth:depth errorType:errorType];
+    NSArray<BugsnagThread *> *threads = [BugsnagThread threadsFromArray:threadDict binaryImages:binaryImages];
 
     BugsnagThread *errorReportingThread = nil;
     for (BugsnagThread *thread in threads) {

Bugsnag/Payload/BugsnagStackframe+Private.h

@@ -14,7 +14,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 + (NSArray<BugsnagStackframe *> *)stackframesWithBacktrace:(uintptr_t *)backtrace length:(NSUInteger)length;
 
-/// Constructs a stackframe object from a stackframe dictionary and list of images captured by KSCrash.
+/// Constructs a stackframe object from a KSCrashReport backtrace dictionary.
 + (nullable instancetype)frameFromDict:(NSDictionary<NSString *, id> *)dict withImages:(NSArray<NSDictionary<NSString *, id> *> *)binaryImages;
 
 /// Constructs a stackframe object from a JSON object (typically loaded from disk.)

Bugsnag/Payload/BugsnagStackframe.m

@@ -10,6 +10,7 @@
 
 #import "BSGKeys.h"
 #import "BSG_KSBacktrace.h"
+#import "BSG_KSCrashReportFields.h"
 #import "BSG_KSMachHeaders.h"
 #import "BSG_Symbolicate.h"
 #import "BugsnagCollections.h"
@@ -29,7 +30,7 @@ @implementation BugsnagStackframe
 
 static NSDictionary * _Nullable FindImage(NSArray *images, uintptr_t addr) {
     for (NSDictionary *image in images) {
-        if ([(NSNumber *)image[BSGKeyImageAddress] unsignedLongValue] == addr) {
+        if ([(NSNumber *)image[@ BSG_KSCrashField_ImageAddress] unsignedLongValue] == addr) {
             return image;
         }
     }
@@ -65,7 +66,7 @@ + (NSNumber *)readInt:(NSDictionary *)json key:(NSString *)key {
 }
 
 + (instancetype)frameFromDict:(NSDictionary<NSString *, id> *)dict withImages:(NSArray<NSDictionary<NSString *, id> *> *)binaryImages {
-    NSNumber *frameAddress = dict[BSGKeyInstructionAddress];
+    NSNumber *frameAddress = dict[@ BSG_KSCrashField_InstructionAddr];
     if (frameAddress.unsignedLongLongValue == 1) {
         // We sometimes get a frame address of 0x1 at the bottom of the call stack.
         // It's not a valid stack frame and causes E2E tests to fail, so should be ignored.
@@ -74,18 +75,18 @@ + (instancetype)frameFromDict:(NSDictionary<NSString *, id> *)dict withImages:(N
 
     BugsnagStackframe *frame = [BugsnagStackframe new];
     frame.frameAddress = frameAddress;
-    frame.symbolAddress = dict[BSGKeySymbolAddress];
-    frame.machoLoadAddress = dict[BSGKeyObjectAddress];
-    frame.machoFile = dict[BSGKeyObjectName];
-    frame.method = dict[BSGKeySymbolName];
+    frame.machoFile = dict[@ BSG_KSCrashField_ObjectName]; // last path component
+    frame.machoLoadAddress = dict[@ BSG_KSCrashField_ObjectAddr];
+    frame.method = dict[@ BSG_KSCrashField_SymbolName];
+    frame.symbolAddress = dict[@ BSG_KSCrashField_SymbolAddr];
     frame.isPc = [dict[BSGKeyIsPC] boolValue];
     frame.isLr = [dict[BSGKeyIsLR] boolValue];
 
     NSDictionary *image = FindImage(binaryImages, (uintptr_t)frame.machoLoadAddress.unsignedLongLongValue);
     if (image != nil) {
-        frame.machoUuid = image[BSGKeyUuid];
-        frame.machoVmAddress = image[BSGKeyImageVmAddress];
-        frame.machoFile = image[BSGKeyName];
+        frame.machoFile = image[@ BSG_KSCrashField_Name]; // full path
+        frame.machoUuid = image[@ BSG_KSCrashField_UUID];
+        frame.machoVmAddress = image[@ BSG_KSCrashField_ImageVmAddress];
     } else if (frame.isPc) {
         // If the program counter's value isn't in any known image, the crash may have been due to a bad function pointer.
         // Ignore these frames to prevent the dashboard grouping on the address.
@@ -112,7 +113,7 @@ + (instancetype)frameFromDict:(NSDictionary<NSString *, id> *)dict withImages:(N
             continue;
         }
 
-        [frames addObject:[[BugsnagStackframe alloc] initWithAddress:address isPc:i == 0]];
+        [frames addObject:[[BugsnagStackframe alloc] initWithAddress:address]];
     }
 
     return frames;
@@ -156,7 +157,6 @@ + (instancetype)frameFromDict:(NSDictionary<NSString *, id> *)dict withImages:(N
         if (match.numberOfRanges != 6) {
             continue;
         }
-        NSString *frameNumber = [string substringWithRange:[match rangeAtIndex:1]];
         NSString *imageName = [string substringWithRange:[match rangeAtIndex:2]];
         NSString *frameAddress = [string substringWithRange:[match rangeAtIndex:3]];
         NSRange symbolNameRange = [match rangeAtIndex:5];
@@ -170,7 +170,7 @@ + (instancetype)frameFromDict:(NSDictionary<NSString *, id> *)dict withImages:(N
             sscanf(frameAddress.UTF8String, "%lx", &address);
         }
 
-        BugsnagStackframe *frame = [[BugsnagStackframe alloc] initWithAddress:address isPc:[frameNumber isEqualToString:@"0"]];
+        BugsnagStackframe *frame = [[BugsnagStackframe alloc] initWithAddress:address];
         frame.machoFile = imageName;
         frame.method = symbolName ?: frameAddress;
         [frames addObject:frame];
@@ -179,10 +179,9 @@ + (instancetype)frameFromDict:(NSDictionary<NSString *, id> *)dict withImages:(N
     return [NSArray arrayWithArray:frames];
 }
 
-- (instancetype)initWithAddress:(uintptr_t)address isPc:(BOOL)isPc {
+- (instancetype)initWithAddress:(uintptr_t)address {
     if ((self = [super init])) {
         _frameAddress = @(address);
-        _isPc = isPc;
         _needsSymbolication = YES;
         BSG_Mach_Header_Info *header = bsg_mach_headers_image_at_address(address);
         if (header) {
@@ -228,12 +227,8 @@ - (NSDictionary *)toDictionary {
     dict[BSGKeySymbolAddr] = FormatMemoryAddress(self.symbolAddress);
     dict[BSGKeyMachoLoadAddr] = FormatMemoryAddress(self.machoLoadAddress);
     dict[BSGKeyMachoVMAddress] = FormatMemoryAddress(self.machoVmAddress);
-    if (self.isPc) {
-        dict[BSGKeyIsPC] = @(self.isPc);
-    }
-    if (self.isLr) {
-        dict[BSGKeyIsLR] = @(self.isLr);
-    }
+    dict[BSGKeyIsPC] = self.isPc ? @YES : nil;
+    dict[BSGKeyIsLR] = self.isLr ? @YES : nil;
     dict[BSGKeyType] = self.type;
     dict[@"codeIdentifier"] = self.codeIdentifier;
     dict[@"columnNumber"] = self.columnNumber;

Bugsnag/Payload/BugsnagThread+Private.h

@@ -30,20 +30,15 @@ NS_ASSUME_NONNULL_BEGIN
 
 @property (readwrite, nonatomic) BOOL errorReportingThread;
 
-+ (NSDictionary *)enhanceThreadInfo:(NSDictionary *)thread
-                              depth:(NSUInteger)depth
-                          errorType:(nullable NSString *)errorType;
++ (NSDictionary *)enhanceThreadInfo:(NSDictionary *)thread;
 
 #if BSG_HAVE_MACH_THREADS
 + (nullable instancetype)mainThread;
 #endif
 
 + (NSMutableArray *)serializeThreads:(nullable NSArray<BugsnagThread *> *)threads;
 
-+ (NSMutableArray<BugsnagThread *> *)threadsFromArray:(NSArray *)threads
-                                         binaryImages:(NSArray *)binaryImages
-                                                depth:(NSUInteger)depth
-                                            errorType:(nullable NSString *)errorType;
++ (NSMutableArray<BugsnagThread *> *)threadsFromArray:(NSArray *)threads binaryImages:(NSArray *)binaryImages;
 
 - (NSDictionary *)toDictionary;
 

Bugsnag/Payload/BugsnagThread.m

@@ -150,57 +150,51 @@ + (NSMutableArray *)serializeThreads:(NSArray<BugsnagThread *> *)threads {
 /**
  * Deerializes Bugsnag Threads from a KSCrash report
  */
-+ (NSMutableArray<BugsnagThread *> *)threadsFromArray:(NSArray *)threads
-                                         binaryImages:(NSArray *)binaryImages
-                                                depth:(NSUInteger)depth
-                                            errorType:(NSString *)errorType {
++ (NSMutableArray<BugsnagThread *> *)threadsFromArray:(NSArray *)threads binaryImages:(NSArray *)binaryImages {
     NSMutableArray *bugsnagThreads = [NSMutableArray new];
 
     for (NSDictionary *thread in threads) {
-        NSDictionary *threadInfo = [self enhanceThreadInfo:thread depth:depth errorType:errorType];
+        NSDictionary *threadInfo = [self enhanceThreadInfo:thread];
         BugsnagThread *obj = [[BugsnagThread alloc] initWithThread:threadInfo binaryImages:binaryImages];
         [bugsnagThreads addObject:obj];
     }
     return bugsnagThreads;
 }
 
 /**
- * Enhances the thread information recorded by KSCrash. Specifically, this will trim the error reporting thread frames
- * by the `depth` configured, and add information to each frame indicating whether they
- * are within the program counter/link register.
- *
- * The error reporting thread is the thread on which the error occurred, and is given more
- * prominence in the Bugsnag Dashboard - therefore we enhance it with extra info.
- *
- * @param thread the captured thread
- * @param depth the 'depth'. This is equivalent to the number of frames which should be discarded from a report,
- * and is configurable by the user.
- * @param errorType the type of error as recorded by KSCrash (e.g. mach, signal)
- * @return the enhanced thread information
+ * Adds isPC and isLR values to a KSCrashReport thread dictionary.
  */
-+ (NSDictionary *)enhanceThreadInfo:(NSDictionary *)thread
-                              depth:(NSUInteger)depth
-                          errorType:(NSString *)errorType {
++ (NSDictionary *)enhanceThreadInfo:(NSDictionary *)thread {
     NSArray *backtrace = thread[@"backtrace"][@"contents"];
     BOOL isReportingThread = [thread[@"crashed"] boolValue];
 
     if (isReportingThread) {
-        BOOL stackOverflow = [thread[@"stack"][@"overflow"] boolValue];
-        NSUInteger seen = 0;
+        NSDictionary *registers = thread[@ BSG_KSCrashField_Registers][@ BSG_KSCrashField_Basic];
+#if TARGET_CPU_ARM || TARGET_CPU_ARM64
+        NSNumber *pc = registers[@"pc"];
+        NSNumber *lr = registers[@"lr"];
+#elif TARGET_CPU_X86
+        NSNumber *pc = registers[@"eip"];
+        NSNumber *lr = nil;
+#elif TARGET_CPU_X86_64
+        NSNumber *pc = registers[@"rip"];
+        NSNumber *lr = nil;
+#else
+#error Unsupported CPU architecture
+#endif
+
         NSMutableArray *stacktrace = [NSMutableArray array];
 
         for (NSDictionary *frame in backtrace) {
             NSMutableDictionary *mutableFrame = [frame mutableCopy];
-            if (seen++ >= depth) {
-                // Mark the frame so we know where it came from
-                if (seen == 1 && !stackOverflow) {
-                    mutableFrame[BSGKeyIsPC] = @YES;
-                }
-                if (seen == 2 && !stackOverflow && [@[BSGKeySignal, BSGKeyMach] containsObject:errorType]) {
-                    mutableFrame[BSGKeyIsLR] = @YES;
-                }
-                [stacktrace addObject:mutableFrame];
+            NSNumber *instructionAddress = frame[@ BSG_KSCrashField_InstructionAddr]; 
+            if ([instructionAddress isEqual:pc]) {
+                mutableFrame[BSGKeyIsPC] = @YES;
+            }
+            if ([instructionAddress isEqual:lr]) {
+                mutableFrame[BSGKeyIsLR] = @YES;
             }
+            [stacktrace addObject:mutableFrame];
         }
         NSMutableDictionary *mutableBacktrace = [thread[@"backtrace"] mutableCopy];
         mutableBacktrace[@"contents"] = stacktrace;

Bugsnag/include/Bugsnag/BugsnagStackframe.h

@@ -43,7 +43,7 @@ BUGSNAG_EXTERN
 @property (strong, nullable, nonatomic) NSNumber *frameAddress;
 
 /**
- * The VM address of the Mach-O file
+ * The Mach-O file's desired base virtual memory address
  */
 @property (strong, nullable, nonatomic) NSNumber *machoVmAddress;
 
@@ -53,17 +53,17 @@ BUGSNAG_EXTERN
 @property (strong, nullable, nonatomic) NSNumber *symbolAddress;
 
 /**
- * The load address of the Mach-O file
+ * The address at which the Mach-O file is mapped into memory
  */
 @property (strong, nullable, nonatomic) NSNumber *machoLoadAddress;
 
 /**
- * Whether the frame was within the program counter
+ * True if `frameAddress` is equal to the value of the program counter register.
  */
 @property (nonatomic) BOOL isPc;
 
 /**
- * Whether the frame was within the link register
+ * True if `frameAddress` is equal to the value of the link register.
  */
 @property (nonatomic) BOOL isLr;
 

CHANGELOG.md

@@ -1,6 +1,13 @@
 Changelog
 =========
 
+## TBD
+
+### Bug fixes
+
+* Fix accuracy of `isLR` and `isPC` stack frame values.
+  [#1470](https://github.com/bugsnag/bugsnag-cocoa/pull/1470)
+
 ## 6.22.2 (2022-08-17)
 
 ### Bug fixes

Tests/BugsnagTests/BugsnagErrorTest.m

@@ -111,9 +111,7 @@ - (BugsnagThread *)findErrorReportingThread:(NSDictionary *)event {
     NSArray *binaryImages = event[@"binary_images"];
     NSArray *threadDict = [event valueForKeyPath:@"crash.threads"];
     NSArray<BugsnagThread *> *threads = [BugsnagThread threadsFromArray:threadDict
-                                                           binaryImages:binaryImages
-                                                                  depth:0
-                                                              errorType:@"user"];
+                                                           binaryImages:binaryImages];
     for (BugsnagThread *thread in threads) {
         if (thread.errorReportingThread) {
             return thread;

Tests/BugsnagTests/BugsnagStackframeTest.m

@@ -238,7 +238,6 @@ - (void)testRealCallStackSymbols {
     NSArray<NSString *> *callStackSymbols = [NSThread callStackSymbols];
     NSArray<BugsnagStackframe *> *stackframes = [BugsnagStackframe stackframesWithCallStackSymbols:callStackSymbols];
     XCTAssertEqual(stackframes.count, callStackSymbols.count, @"All valid stack frame strings should be parsed");
-    XCTAssertTrue(stackframes.firstObject.isPc, @"The first stack frame should have isPc set to true");
     BOOL __block didSeeMain = NO;
     [stackframes enumerateObjectsUsingBlock:^(BugsnagStackframe *stackframe, NSUInteger idx, BOOL *stop) {
         XCTAssertNotNil(stackframe.frameAddress);