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

Emit trivial leaf CFI for UnwindOp::None on arm64 #619

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Jul 6, 2022

fixes #618

This needs some testing on real inputs

@mstange
Copy link
Contributor

mstange commented Jul 6, 2022

With this patch, dump_syms produces the following output for libsystem_kernel.dylib from macOS 12.3.1: https://gist.github.com/mstange/8fdff669c0785ec7a4c005311e639801

@Gankra
Copy link
Contributor Author

Gankra commented Jul 6, 2022

Before:

STACK CFI INIT 50e4 70 .cfa: x29 16 + x29: .cfa -16 + ^ .ra: .cfa -8 + ^ x19: .cfa -24 + ^ x20: .cfa -32 + ^ x21: .cfa -40 + ^ x22: .cfa -48 + ^
STACK CFI INIT 52e0 30 .cfa: x29 16 + x29: .cfa -16 + ^ .ra: .cfa -8 + ^
STACK CFI INIT 5368 4 .cfa: sp 0 + .ra: x30 0 +

After:

STACK CFI INIT 50e4 70 .cfa: x29 16 + x29: .cfa -16 + ^ .ra: .cfa -8 + ^ x19: .cfa -24 + ^ x20: .cfa -32 + ^ x21: .cfa -40 + ^ x22: .cfa -48 + ^
STACK CFI INIT 5154 18c .cfa: sp .ra: x30
STACK CFI INIT 52e0 30 .cfa: x29 16 + x29: .cfa -16 + ^ .ra: .cfa -8 + ^
STACK CFI INIT 5310 58 .cfa: sp .ra: x30
STACK CFI INIT 5368 4 .cfa: sp 0 + .ra: x30 0 +

Resulting socc-pair diff:

       frames: [
         0: {
           file: null
           frame: 0
           function: __psynch_cvwait
           function_offset: 0x00000008
           line: null
           missing_symbols: false
           module: libsystem_kernel.dylib
           module_offset: 0x00005290
           offset: 0x1a2704290
           trust: context
           unloaded_modules: null
         }
         1: {
           file: null
           frame: 1
           function: _pthread_cond_wait
           function_offset: 0x000004d0
           line: null
           missing_symbols: false
           module: libsystem_pthread.dylib
           module_offset: 0x00007838
           offset: 0x1a273e838
~          local had better trust (frame_pointer vs cfi)
           unloaded_modules: null
         }
-        local had extra array value:
         2: {
           file: hg:hg.mozilla.org/mozilla-central:mozglue/misc/ConditionVariable_posix.cpp:8e5247451c9a83abe6f877f5f2c62332a84c9aaf
           frame: 2
           function: mozilla::detail::ConditionVariableImpl::wait(mozilla::detail::MutexImpl&)
           function_offset: 0x0000000000000008
           line: 106
           missing_symbols: false
           module: libmozglue.dylib
           module_offset: 0x0000000000054f08
           offset: 0x0000000104e94f08
           trust: cfi
           unloaded_modules: null
         }
         2: {
           file: hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:8e5247451c9a83abe6f877f5f2c62332a84c9aaf
~          ignoring field frame: 2
           function: nsThread::ProcessNextEvent(bool, bool*)
           function_offset: 0x00000420
           line: 1141
           missing_symbols: false
           module: XUL
           module_offset: 0x003fa62c
           offset: 0x11a6a262c
           trust: cfi
           unloaded_modules: null
         }

@Gankra Gankra marked this pull request as ready for review July 6, 2022 21:39
@Gankra Gankra requested a review from a team July 6, 2022 21:39
@Gankra
Copy link
Contributor Author

Gankra commented Jul 6, 2022

  • Before this change or my arm fp/lr rewrites: we were failing to use CFI on this frame, using frame pointers badly, and skipping the frame 2 of the stackwalk.

  • WITH my arm rewrites: we still fail to use CFI and end up skipping frame 1 of the stackwalk because we're conservatively assuming this isn't a leaf function

  • WITH this change: we now fully use CFI and get both frame 1 and 2, no frame pointers needed!

@loewenheim
Copy link
Contributor

LGTM, but I would like to wait for a second opinion.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

sounds reasonable. Too bad all these functions don’t have "real" CFI, and you need to apply such heuristics.

@Swatinem Swatinem merged commit 54dde7e into getsentry:master Jul 7, 2022
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.

consider adding the compact-unwind null opcode hack for arm64?
4 participants