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(wasm): WASM support #301

Merged
merged 22 commits into from
Nov 26, 2020
Merged

feat(wasm): WASM support #301

merged 22 commits into from
Nov 26, 2020

Conversation

mitsuhiko
Copy link
Member

@mitsuhiko mitsuhiko commented Nov 24, 2020

This implements WASM support to tackle #300. It uses a slightly modified protocol where a sender can supply an addr_mode to be explicit about the addressing mode. "abs" is the default absolute addressing mode, another option now is rel:index with the index of a module in which case the instruction address is considered to be module relative. This change also now lets existing users benefit of this by providing the relative address within any other debug module as alternative.

@mitsuhiko mitsuhiko requested a review from a team November 24, 2020 19:58
@mitsuhiko mitsuhiko marked this pull request as draft November 24, 2020 19:59
Cargo.toml Outdated Show resolved Hide resolved
@mitsuhiko mitsuhiko marked this pull request as ready for review November 24, 2020 23:45
@mitsuhiko mitsuhiko changed the title feat(wasm): Initial work for WASM support feat(wasm): WASM support Nov 24, 2020
src/actors/symbolication.rs Outdated Show resolved Hide resolved
tests/integration/test_wasm.py Outdated Show resolved Hide resolved
mitsuhiko and others added 2 commits November 25, 2020 14:10
@mitsuhiko
Copy link
Member Author

Alternative proposal would be to send instruction_addr in the format INDEX!ADDR instead of sending INDEX as in_module.

docs/advanced/symbol-server-compatibility.md Show resolved Hide resolved
docs/advanced/symbol-server-compatibility.md Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
@mitsuhiko
Copy link
Member Author

mitsuhiko commented Nov 25, 2020

@jan-auer based on the conversation today about the problem with the address having two modes I tried a few options now and I did not feel happy with most of them.

Things I tried:

  • instruction_addr changes from 0xdeadbeef to a two-variant version. 0xdeadbeef as an absolute one and index!0xdeadbeef to a relative within a module one.
  • The existence of a addr_base_module property changing all addressing modes from absolute to be within-module.
  • having both an absolute_addr and relative_addr attribute for instruction_addr and same for the symbol address.

I ultimately find that from the source code and external API the addr_base_module property to change the addressing mode is the cleanest. The benefit is that the interface does not change for non WASM or relative lookup consumers. Any user of the API using only absolute indexing can rely on always getting absolute addresses back. Only someone who wants to use relative addressing needs to be aware of addr_base_module. It can also come back in the response if an address cannot be made absolute (eg: WASM).

The benefit though is that it's a more robust API because you always get addresses back, worst case they are of a different kind. This is preferred over someone now getting back things that are not addresses at all (eg: 42!0x42).

I have an alternative sitting around that replaces the HexValue with an InstructionAddr with two variants Absolute(u64) and InModule(usize, u64). This however also now means special handling could in theory be necessary for both sym_addr and instruction_addr independently and generally is more complex to comprehend.

The version I like least was two attributes for addresses. That makes everything really confusing and ugly both in code and API.

I ended up changing in_module to addr_base_module though which I think is a better name.

@mitsuhiko
Copy link
Member Author

@jan-auer This is ready for review now I believe.

@mitsuhiko
Copy link
Member Author

Now that I'm also looking at the changes in Sentry I'm thinking we should not serialize out abs as addr_mode but instead leave it at null for abs. Thoughts?

@mitsuhiko
Copy link
Member Author

Actually for symbolicator I think it makes sense to always be explicit and send the addr_mode. The biggest question will be what we want to do on the sentry side about old events without addr_mode and new events with addr_mode.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Thanks! A number of nitpicks, but this is G2G from my end.

docs/advanced/symbol-server-compatibility.md Outdated Show resolved Hide resolved
src/utils/addr.rs Outdated Show resolved Hide resolved
src/actors/symbolication.rs Outdated Show resolved Hide resolved
src/actors/symbolication.rs Show resolved Hide resolved
src/actors/symbolication.rs Outdated Show resolved Hide resolved
src/actors/symbolication.rs Outdated Show resolved Hide resolved
src/actors/symbolication.rs Outdated Show resolved Hide resolved
metric!(counter("relative_addr.underflow") += 1);
return Err(FrameStatus::MissingSymbol);
// get the relative caller address
let relative_addr = if let Some(addr) = lookup_result.relative_addr {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Since SymcacheLookupResult is now a thing, this could become a method on the result type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this now but it takes too many args. Hmm

src/utils/addr.rs Show resolved Hide resolved
@mitsuhiko mitsuhiko merged commit 41b838f into master Nov 26, 2020
@mitsuhiko mitsuhiko deleted the feature/wasm-support branch November 26, 2020 18:04
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.
Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section under the following heading:

  1. Features: For new user-visible functionality.
  2. Bug Fixes: For user-visible bug fixes.
  3. Internal: For features and bug fixes in internal operation.

To the changelog entry, please add a link to this PR (consider a more descriptive message):

- WASM support. ([#301](https://github.com/getsentry/symbolicator/pull/301))

If none of the above apply, you can opt out by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against ecf82f6

Hamled added a commit to Hamled/symbolicator that referenced this pull request Jun 18, 2021
Commit 2a19481 (part of getsentry#301) changed the
`SourceLookup::get_object_index_by_addr` function to return a stable
module index value, independent of the inner Vec's ordering. This
updates the debug sessions collection to be keyed on the same module
index.
Hamled added a commit to Hamled/symbolicator that referenced this pull request Jun 18, 2021
Commit 2a19481 (part of getsentry#301) changed the
`SourceLookup::get_object_index_by_addr` function to return a stable
module index value, independent of the inner Vec's ordering. This
updates the debug sessions collection to be keyed on the same module
index.
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.

4 participants