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

feat: Add field to RawFrame to control instruction address adjustment #948

Merged
merged 10 commits into from
Jan 19, 2023

Conversation

loewenheim
Copy link
Contributor

@loewenheim loewenheim commented Dec 20, 2022

This implements the symbolicator part of getsentry/rfcs#43. It adds a new field instruction_addr_needs_adjustment: Option<bool> to RawFrame which is then used to decide whether the frame's instruction address needs to be adjusted to account for an off-by-one error. Internally, this field is mapped to a new enum AdjustInstructionAddr which should make the intended behavior clearer than using a bare Option<bool>.

Some open questions:

  • The functions AdjustInstructionAddr::default_for_thread and AdjustInstructionAddr::for_frame are very short and only used in one place each; I created dedicated functions for clarity and documentation purposes. Should we inline these instead?
  • What should be the value of instruction_addr_needs_adjustment in the frame returned by symbolicate_native_frame? Should we use the value from the input frame? Should we just use None?

@loewenheim loewenheim self-assigned this Dec 20, 2022
@loewenheim
Copy link
Contributor Author

loewenheim commented Dec 20, 2022

Another question: should we make AdjustInstructionAddr public for documentation visibility?

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.

lgtm

crates/symbolicli/src/main.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022

Codecov Report

Base: 69.38% // Head: 69.41% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (a86841a) compared to base (6c2a4ad).
Patch coverage: 81.08% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
+ Coverage   69.38%   69.41%   +0.02%     
==========================================
  Files          74       74              
  Lines       11540    11573      +33     
==========================================
+ Hits         8007     8033      +26     
- Misses       3533     3540       +7     
Impacted Files Coverage Δ
crates/symbolicator-service/src/types/mod.rs 75.26% <ø> (ø)
crates/symbolicli/src/main.rs 0.21% <0.00%> (-0.01%) ⬇️
...olicator-service/src/services/symbolication/mod.rs 82.40% <83.33%> (-0.09%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@loewenheim loewenheim marked this pull request as ready for review December 20, 2022 14:26
@loewenheim loewenheim requested a review from a team December 20, 2022 14:26
@loewenheim loewenheim enabled auto-merge (squash) December 20, 2022 14:35
@Swatinem Swatinem merged commit 1a94eae into master Jan 19, 2023
@Swatinem Swatinem deleted the ref/return-address branch January 19, 2023 13:29
Swatinem added a commit to getsentry/sentry that referenced this pull request Feb 1, 2023
Implements handling of `instruction_addr_adjustment` and attaching
appropriate per-frame attributes (`adjust_instruction_addr`) when sending stack traces to
symbolicator.

It implements the Sentry and Profiling side of [RFC
43](getsentry/rfcs#43)

The Symbolicator side is implemented in
getsentry/symbolicator#948.

---------

Co-authored-by: Sebastian Zivota <loewenheim@mailbox.org>
mikejihbe pushed a commit to getsentry/sentry that referenced this pull request Feb 6, 2023
Implements handling of `instruction_addr_adjustment` and attaching
appropriate per-frame attributes (`adjust_instruction_addr`) when sending stack traces to
symbolicator.

It implements the Sentry and Profiling side of [RFC
43](getsentry/rfcs#43)

The Symbolicator side is implemented in
getsentry/symbolicator#948.

---------

Co-authored-by: Sebastian Zivota <loewenheim@mailbox.org>
wmak pushed a commit to getsentry/sentry that referenced this pull request Feb 16, 2023
Implements handling of `instruction_addr_adjustment` and attaching
appropriate per-frame attributes (`adjust_instruction_addr`) when sending stack traces to
symbolicator.

It implements the Sentry and Profiling side of [RFC
43](getsentry/rfcs#43)

The Symbolicator side is implemented in
getsentry/symbolicator#948.

---------

Co-authored-by: Sebastian Zivota <loewenheim@mailbox.org>
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