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

Add Trap::trap_code #2309

Merged
merged 4 commits into from
Oct 27, 2020
Merged

Conversation

leoyvens
Copy link
Contributor

The public Trap in wasmtime is right now very opaque compared to the internal information which is available about the trap reason. This exposes the trap code of a wasm trap, which is useful to for example differentiate traps from host functions to traps coming from wasm itself.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Oct 22, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton
Copy link
Member

Thanks for the PR! I don't think we'll want to expose the cranelift-internal TrapCode as part of the public API of the wasmtime crate, though. That's an internal implementation detail of wasmtime, so I think it'd be best to instead have a public #[non_exhaustive] enum perhaps defined in wasmtime itself?

@leoyvens
Copy link
Contributor Author

@alexcrichton That's a good suggestion, for my personal use case I'd prefer that it were exhaustive but I get that for versioning it's best for it to not be.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looking good, thanks! I left some comments about the documentation, but I think you'll need to reexport TrapCode from the top-level to make it accessbile as well. Can you additionally add some tests which assert expected values of the trap code from running wasm?

crates/wasmtime/src/trap.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/trap.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/trap.rs Outdated Show resolved Hide resolved
BadSignature,

/// An integer arithmetic operation caused an overflow.
IntegerOverflow,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this can actually arise from wasm code? We may not need to include this?

Copy link
Contributor Author

@leoyvens leoyvens Oct 27, 2020

Choose a reason for hiding this comment

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

This does happen for non-saturating truncation and this specific case of division https://github.com/WebAssembly/spec/blob/55a5e2c88890ccbe2b992f571b95704234977dac/interpreter/exec/int.ml#L131.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, indeed!

crates/wasmtime/src/trap.rs Outdated Show resolved Hide resolved
@@ -174,6 +251,14 @@ impl Trap {
pub fn trace(&self) -> &[FrameInfo] {
&self.inner.wasm_trace
}

/// Code of a trap that happened while executing a WASM instruction.
Copy link
Member

Choose a reason for hiding this comment

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

Could this be expanded a bit to explain when it's None and when it's Some?

@leoyvens
Copy link
Contributor Author

@alexcrichton Thanks for the speedy review, I've addressed all suggested changes.

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Oct 27, 2020
};
let trap = err.downcast_ref::<Trap>().unwrap();
assert_eq!(trap.trap_code(), Some(TrapCode::MemoryOutOfBounds));
}
Copy link
Member

Choose a reason for hiding this comment

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

Could this get moved to tests/all/*.rs? That's were we store most other tests.

Copy link
Member

Choose a reason for hiding this comment

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

Also, while you're at it, mind simplifying this a bit with a helper function?

fn assert_trap_code(wat: &str, code: ir::TrapCode)

or something like that?

@leoyvens
Copy link
Contributor Author

@alexcrichton Done!

@alexcrichton alexcrichton merged commit bde9555 into bytecodealliance:main Oct 27, 2020
@alexcrichton
Copy link
Member

Thanks again for this!

cfallin pushed a commit that referenced this pull request Nov 30, 2020
* add Trap::trap_code

* Add non-exhaustive wasmtime::TrapCode

* wasmtime: Better document TrapCode

* move and refactor test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants