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

Support for -fwasm-exceptions in wasm-split and others #1490

Conversation

trzeciak
Copy link
Contributor

In this PR I purpose to add exception-handling feature to wasmbin dependencies.

With reference to these issuses:

in order for the symbolizer and wasm-split to support -fwasm-exceptions, is needed to set exception-handling feature in the wasmbin dependency.

PS: Just to be sure, I also asked the author of wasmbin LINK for confirmation that these changes were safe. But looking at the sourcecode it looks like it should be safe.

@trzeciak trzeciak requested a review from a team June 27, 2024 12:36
Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.74%. Comparing base (5a85fd7) to head (3abaa3f).
Report is 109 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1490      +/-   ##
==========================================
- Coverage   76.09%   73.74%   -2.36%     
==========================================
  Files         101      104       +3     
  Lines       15262    15655     +393     
==========================================
- Hits        11613    11544      -69     
- Misses       3649     4111     +462     

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.

thank you, this looks simple enough :-)

@Swatinem Swatinem merged commit 9a581c8 into getsentry:master Jun 27, 2024
11 checks passed
@trzeciak trzeciak deleted the deps/crates/symbolicator/wasm-split/wasmbin branch June 27, 2024 15:06
@trzeciak
Copy link
Contributor Author

It happened so quickly that I didn't even have time to ask what the next steps would be 😄 💪 ⚡

  • What else needs to be done (maybe upgrade the version of the symbolizer itself?)?
  • When can we expect the release of the symbolicator version (I'm not insisting, I'm just asking about the flow out of curiosity)?
  • How to check (if possible) which symbolicator version is used on the sentry backed/cloud side?
  • When can we expect this version of the symbolicator to be in sentry backend/cloud?

Thank you again for handling this PR so quickly.

@loewenheim
Copy link
Contributor

This change in itself has no effect on Symbolicator (the symbolication server that runs as part of Sentry and that you can also run yourself). wasm-split is a separate tool that is intended for users to run themselves. I'm not sure if further changes are necessary for Symbolicator to process the resulting wasm files—do you have a file to test this with?

As for releases, Symbolicator is released monthly; the last release was on 18 Jun. You can compile and run wasm-split from master in the meantime.

@trzeciak
Copy link
Contributor Author

This change in itself has no effect on Symbolicator

That's interesting, I had a different impression, but I haven't no prove yet xD. I have very little experience with rust and that's why.

the symbolication server that runs as part of Sentry and that you can also run yourself

It is also intresting, maybe I will test something localy, nice!

I'm not sure if further changes are necessary for Symbolicator to process the resulting wasm files—do you have a file to test this with?

I have something in mind, but I still need time for a few tests. Considering how good communication works here, I won't give up on it.

Great news, thank you for your help!

@loewenheim
Copy link
Contributor

My pleasure, please let us know if we can assist you further.

@trzeciak
Copy link
Contributor Author

I found something that already reported, and I updated those issue 1203 in sentry-cli (I not link it directly because are not related to this one).

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