Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KSCrashReportFilterAppleFmt now shows correct architecture in header and libraries list (arm64e) #415

Merged
merged 2 commits into from Nov 26, 2023
Merged

KSCrashReportFilterAppleFmt now shows correct architecture in header and libraries list (arm64e) #415

merged 2 commits into from Nov 26, 2023

Conversation

nacho4d
Copy link
Contributor

@nacho4d nacho4d commented Sep 4, 2021 •
edited

List of changes:

  • Fixes AppleFormatReport binary Images arch is wrong?  #403 architecture name in header and list of loaded libraries are now correct. See I am fixing method -[KSCrashReportFilterAppleFmt CPUType:isSystemInfoHeader:] and -[KSCrashReportFilterAppleFmt CPUArchForMajor: minor:]. In one case I use NXGetArchInfoFromCpuType as that is the reliable way to get architecture names from major and minor cpu types.

  • Removed various #ifdef CPU_TYPE_ARM64 because now a days that is always defined in system frameworks. It improves source readability.

  • Various minor aesthetic fixes to resemble better Apple crash reports:

    • General sorting method for registers. This works and arm64 and should work for unknown architectures too.
    • Fixes indentation of various items in header and addresses in libraries list.
    • Adds other hardocoded items in header to resemble more (Role, Parent Process, )

After this fix you can see there very few differences between KSCrash report and Apple crash report (Xcode12.5.1)

LeftApple-RightKSCrash

LeftApple-RightKSCrash2

@nacho4d nacho4d mentioned this pull request Sep 4, 2021
Closed
@nacho4d nacho4d changed the title KSCrashReportFilterAppleFmt now shows correct architecture in header (arch in list of libraries still incorrect for arm64e) KSCrashReportFilterAppleFmt now shows correct architecture in header and libraries list (arm64e) Sep 4, 2021
@nacho4d
Copy link
Contributor Author

nacho4d commented Sep 6, 2021

@kstenerud Could you review this please?

@nacho4d
Copy link
Contributor Author

nacho4d commented Dec 16, 2021

@kstenerud @nickh-apadmi Could you take a look at this please?

bamx23
bamx23 requested changes Nov 12, 2023
View reviewed changes
Copy link
Collaborator

@bamx23 bamx23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @nacho4d, thanks for your PR. Sorry for a delay. If possible, could you address several comments I left?

Source/KSCrash/Reporting/Filters/KSCrashReportFilterAppleFmt.m Outdated
Comment on lines 533 to 534
//NSString* isBaseImage = (path && [executablePath isEqualToString:path]) ? @"+" : @" ";
NSString* isBaseImage = @"";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this argument from format instead of using ""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will fix this!

Source/KSCrash/Reporting/Filters/KSCrashReportFilterAppleFmt.m Outdated
Comment on lines 473 to 508
NSDictionary* system = [self systemReport:report];
NSString* executablePath = [system objectForKey:@KSCrashField_ExecutablePath];
//NSDictionary* system = [self systemReport:report];
//NSString* executablePath = [system objectForKey:@KSCrashField_ExecutablePath];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove it instead of commenting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will fix this!

Source/KSCrash/Reporting/Filters/KSCrashReportFilterAppleFmt.m Outdated
Comment on lines 268 to 270
// In Apple platforms we can use this nice function to get the name of a particular architecture
const NXArchInfo* info = NXGetArchInfoFromCpuType(majorCode, minorCode);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should #ifdef __APPLE__ be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will fix this!

nacho4d
nacho4d commented Nov 22, 2023
View reviewed changes
Copy link
Contributor Author

@nacho4d nacho4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking at my PR. I agree on all comments. I will try to fix this this week.

Source/KSCrash/Reporting/Filters/KSCrashReportFilterAppleFmt.m Outdated
Comment on lines 268 to 270
// In Apple platforms we can use this nice function to get the name of a particular architecture
const NXArchInfo* info = NXGetArchInfoFromCpuType(majorCode, minorCode);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will fix this!

Source/KSCrash/Reporting/Filters/KSCrashReportFilterAppleFmt.m Outdated
Comment on lines 473 to 508
NSDictionary* system = [self systemReport:report];
NSString* executablePath = [system objectForKey:@KSCrashField_ExecutablePath];
//NSDictionary* system = [self systemReport:report];
//NSString* executablePath = [system objectForKey:@KSCrashField_ExecutablePath];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will fix this!

Source/KSCrash/Reporting/Filters/KSCrashReportFilterAppleFmt.m Outdated
Comment on lines 533 to 534
//NSString* isBaseImage = (path && [executablePath isEqualToString:path]) ? @"+" : @" ";
NSString* isBaseImage = @"";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will fix this!

@nacho4d
KSCrashReportFilterAppleFmt now shows correct architecture for arm64e…
0746956 
…. Various minor aesthetics changes to resemble better Apple crash reports as Xcode12.
@nacho4d
Remove comments from apple format
ba0ae49
@nacho4d
Copy link
Contributor Author

nacho4d commented Nov 25, 2023

@bamx23 I have fixed all comments!
Review it at your earliest availability please.

bamx23
bamx23 approved these changes Nov 26, 2023
View reviewed changes
Copy link
Collaborator

@bamx23 bamx23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments and for the contribution!

@bamx23 bamx23 merged commit cdc4d20 into kstenerud:master Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bamx23 bamx23 bamx23 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AppleFormatReport binary Images arch is wrong?
2 participants
@nacho4d @bamx23