Stream: git-wasmtime

Topic: wasmtime / PR #2309 Add Trap::trap_code

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2020 at 15:49):

leoyvens opened PR #2309 from trap-is-user to main:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2020 at 21:24):

leoyvens updated PR #2309 from trap-is-user to main:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2020 at 21:25):

leoyvens updated PR #2309 from trap-is-user to main:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 03:32):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 03:32):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 03:32):

alexcrichton created PR Review Comment:

I think this documentation may need an update since I think it's cranelift-specific. Also can you add a test that traps are reported as HeapOutOfBounds? The extra clause here is a little worrisome...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 03:32):

alexcrichton created PR Review Comment:

This I presume is copy-pasted, but mind dropping this clause? I believe this is no longer the case.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 03:32):

alexcrichton created PR Review Comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 03:32):

alexcrichton created PR Review Comment:

I think the table_addr bit here needs to be tweaked in the documentation since that's cranelift-internal.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 03:32):

alexcrichton created PR Review Comment:

The "resumable" part here can be dropped since that's not true of wasmtime right now.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 03:32):

alexcrichton created PR Review Comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 17:00):

leoyvens updated PR #2309 from trap-is-user to main:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 17:01):

leoyvens submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 17:01):

leoyvens created PR Review Comment:

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. Maybe there is a better name for this variant.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 17:01):

leoyvens edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 17:02):

leoyvens submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 17:02):

leoyvens created PR Review Comment:

Added tests, whatever that clause means I couldn't find any consequences of it in testing so I removed it here.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 19:48):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 19:48):

alexcrichton created PR Review Comment:

Ah yes, indeed!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 19:48):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 19:48):

alexcrichton created PR Review Comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 19:49):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 19:49):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 19:49):

alexcrichton created PR Review Comment:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 20:44):

leoyvens updated PR #2309 from trap-is-user to main:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 21:30):

alexcrichton merged PR #2309.

Last updated: Nov 22 2024 at 17:03 UTC