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

Provided context for exception's thread for mac #39

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

saf-e
Copy link

@saf-e saf-e commented Apr 2, 2024

Collect exception's thread context to give ability traverse call stack

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.

I will defer this to @supervacuus

src/client/mac/handler/exception_handler.h Show resolved Hide resolved
@supervacuus
Copy link
Collaborator

Hi @saf-e!

Thanks for the contribution! Can you elaborate on what you're trying to achieve with this change? As @Swatinem mentioned in #39 (comment), this not only breaks compatibility with the upstream project but also breaks the current interface with the Native SDK.

For this change to make sense in the Native SDK context, the retrieved information would have to be passed on somehow to the client via the on_crash hook. To do so, we'd have to retrieve the context for this hook on all backends and all platforms. But since you didn't provide a PR on the Native SDK side, I cannot imagine what context you'd use this change in.

The change, as it currently exists in this PR, does not provide any value to the Native SDK users. If you can provide a few lines of explanation, that would help. Thanks.

@saf-e
Copy link
Author

saf-e commented Apr 15, 2024

As I already mentioned, breakpad does not provide any exception context to the callback for mac (for windows we have it). So, its not possible make any logic on it.

In our case, we use exception's callstack (obtained from context), to analyze do we want to sent this crash to sentry or not

@supervacuus
Copy link
Collaborator

Thanks, @saf-e. I probably formulated my question incorrectly: I understand what the feature you're trying to add does. The question for me is how you want to integrate this. If I merge this as is, the breakpad fork will be unusable in the Native SDK (because the build will break).

So, my question boils down to this:

  • do you plan to create a PR for the Native SDK, too?
  • if so, do you have the capacity/time to make this a non-breaking change vs. vanilla breakpad (by adding interfaces instead of changing existing ones and then adapting the compile-time conditionals in the breakpad-backend to support both)?

I am okay with taking over the integration, but it will take a while until I have time to do this.

@saf-e
Copy link
Author

saf-e commented Apr 16, 2024

do you plan to create a PR for the Native SDK, too?

yes, but it has no sense w/o this one, so it waits for its turn
if you know some github's multi-commit feature, I'll gladly use it

by adding interfaces instead of changing existing ones and then adapting the compile-time conditionals in the breakpad-backend to support both

I suppose thats possible, will try to do in nearest future

@supervacuus
Copy link
Collaborator

do you plan to create a PR for the Native SDK, too?

yes, but it has no sense w/o this one, so it waits for its turn if you know some github's multi-commit feature, I'll gladly use it

You can checkout the breakpad submodule at the commit in this PR and then commit it to a PR in the Native SDK. Github PRs have no problem with submodules as long as the referenced commit (i.e., its branch/fork) is accessible via a GitHub remote (like from a PR of the remote in .gitmodules). It is a very convenient feature, and it makes changes to the submodules much easier to review.

by adding interfaces instead of changing existing ones and then adapting the compile-time conditionals in the breakpad-backend to support both

I suppose thats possible, will try to do in nearest future

Thx!

@saf-e
Copy link
Author

saf-e commented Jun 3, 2024

@supervacuus added ifdef for sentry compilation, plz check if you are ok with the approach

@supervacuus
Copy link
Collaborator

Sorry for the gigantic delay, @saf-e. Thanks for the effort. This will be integrated via getsentry/sentry-native#1083

@supervacuus supervacuus merged commit 43812d6 into getsentry:handler Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants