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.

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

[PLAT-7335] Use Function Starts for symbolication #1214

Merged
merged 7 commits into from
Oct 29, 2021

Conversation

nickdowell
Copy link
Contributor

@nickdowell nickdowell commented Oct 20, 2021

Goal

Prevent incorrect function names appearing in stack traces.

The symbols found by dladdr() and bsg_ksdldladdr() can be very misleading when run against binaries that have a mix of functions with and without symbols.

dladdr() and bsg_ksdldladdr() scan through the symbol table looking for the symbol with highest address that precedes the instruction address. The symbol table does not contain information about the sizes of the functions the symbols relate to.

For an instruction address inside a hidden function, the symbol returned will be that of a function that precedes it in memory.

This can result in a nonsensical looking stack trace if a dSYM has not been uploaded to Bugsnag.

If a dSYM has been uploaded, the back-end performs its own (more accurate) symbolication and the nonsensical looking stack trace will not be seen.

Design

The Function Starts list, which is in the __LINKEDIT segment of binaries built with Xcode 4 and later, describes the addresses of all the functions in the binary, whether hidden or not, and can be used to improve accuracy by:

  • Scanning through the function starts for a function that contains the instruction address.
  • Only If the function address is found, scanning the symbol table for the best entry that matches the address.

Changeset

bsg_ksdldladdr() has been replaced with bsg_symbolicate() and struct bsg_symbolicate_result is now used throughout instead of Dl_info... Because the behaviour no longer matches that of dladdr(), it could be confusing to continue using the existing function name or struct.

Note: the stack frame's symbolAddress is now subtly different, containing the address of the function rather than that of the closest symbol found. This should allow the back-end to symbolicate more accurately using the system dSYMs.

Uses CALL_INSTRUCTION_FROM_RETURN_ADDRESS to improve accuracy when symbolicating handled errors, following the logic in bsg_ksbt_symbolicate() used for unhandled errors.

Testing

Existing E2E scenarios and unit tests (BugsnagStackframeTest and BugsnagThreadTests) validate the symbolication results, and continue to pass. See this full run.

An E2E test on another project that incorporates bugsnag-cocoa has verified that invalid function names no longer appear in stack traces.

Manually tested on 32-bit armv7 device to verify that symbolication works there (for thumb instructions.)

Tested performance of bsg_kscrash_i_onCrash() and found that this change has little to no impact - adding a millisecond or two to the 150ms measured on an iPad3,1

@nickdowell nickdowell changed the base branch from master to next October 20, 2021 15:08
@github-actions
Copy link

github-actions bot commented Oct 20, 2021

Infer: No issues found 🎉

OCLint: No issues found 🎉

Bugsnag.framework binary size decreased by 176 bytes from 1,255,032 to 1,254,856 🎉

Generated by 🚫 Danger

@nickdowell nickdowell marked this pull request as ready for review October 28, 2021 08:27
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

Very cool - were you aiming for adding and removing exactly the same number of lines though??

@nickdowell nickdowell merged commit ad54051 into next Oct 29, 2021
@nickdowell nickdowell deleted the nickdowell/function-starts branch October 29, 2021 07:07
@nickdowell nickdowell mentioned this pull request Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants