Conversation
jbourassa
force-pushed
the
trap
branch
5 times, most recently
from
5054ef7 to
7c5da67
Compare
jbourassa
left a comment
jbourassa
left a comment
There was a problem hiding this comment.
The mem:check task found issues:
==11329== valgrind: Unrecognised instruction at address 0x5829004. ==11329== Your program just tried to execute an instruction that > Valgrind ==11329== did not recognise. There are two possible reasons for this. ==11329== 1. Your program has a bug and erroneously jumped to a non-code ==11329== location. If you are running Memcheck and you just saw a ==11329== warning about a bad jump, it's probably your program's fault. ==11329== 2. The instruction is legitimate but Valgrind doesn't handle it, ==11329== i.e. it's Valgrind's fault. If you think this is the case or ==11329== you are not sure, please let us know and we'll try to fix it. ==11329== Either way, Valgrind will now raise a SIGILL signal which will ==11329== probably kill your program.
After tracking this down, I found out that it happens in Wasmtime on trap: a pure Rust program running a Wasm module that traps will trigger the same error. Note that it also happens on the latest valgrind, I tried a fresh copy from source (3.20)
I'm not sure what to do with this besides filing an issue on Wasmtime and moving on.
ext/Cargo.toml
Outdated
| magnus = { git = "https://github.com/matsadler/magnus", features = ["rb-sys-interop"] } | ||
| rb-sys = "~0.9.38" | ||
| wasmtime = "2.0.2" | ||
| anyhow = "1.*.*" |
There was a problem hiding this comment.
This is to define a function that takes a anyhow::Error from Wasmtime. I picked 1.*.* to be very loose and hopefully stay in sync with Wasmtime's version.
lib/wasmtime.rb
Outdated
| STACK_OVERFLOW = :stack_overflow | ||
| HEAP_MISALIGNED = :heap_misaligned | ||
| TABLE_OUT_OF_BOUNDS = :table_out_of_bounds | ||
| INDIRECT_CALL_TO_NULL = :indirect_call_to_null | ||
| BAD_SIGNATURE = :bad_signature | ||
| INTEGER_OVERFLOW = :integer_overflow | ||
| INTEGER_DIVISION_BY_ZERO = :integer_division_by_zero | ||
| BAD_CONVERSION_TO_INTEGER = :bad_conversion_to_integer | ||
| UNREACHABLE_CODE_REACHED = :unreachable_code_reached | ||
| INTERRUPT = :interrupt | ||
| ALWAYS_TRAP_ADAPTER = :always_trap_adapter | ||
| UNKNOWN = :unknown |
There was a problem hiding this comment.
Having those defined as Ruby here means YARD can see them. I don't have a mean in yard-rustdoc to define constants like these.
ext/src/ruby_api/errors.rs
Outdated
| store | ||
| .context_mut() | ||
| .data_mut() | ||
| .take_last_error() |
There was a problem hiding this comment.
Getting into a tangent here; but this method hints -- at least to me -- that we are storing multiple errors and just taking the last one, when in reality we are just storing one, the last occurrence. I'd prefer if we could rename this to simply take_error. Doesn't have to be done in this PR though.
We used to handle the Wasm error from 3 different places. As I'm going change how we handle errors, I figured I might as well start by refactoring that code.
| } | ||
| } | ||
| } | ||
| impl<T: TypedData> Copy for WrappedStruct<T> {} |
There was a problem hiding this comment.
Don't see a reason why not, under the (transparent) wrappers it's basically a VALUE. It removed the need to clone() in a couple places.
|
Re the |
The perfect solution would've had the following properties: - Self-documenting - Protect against typos (e.g. not just symbols) - Can be used in with `case` (e.g. not just methods) - Easy to keep in sync between Rust and Ruby This is the closest I could come that ticked most of these boxes. The current solution's downside is that if you add a trap code, you need to go and update the Ruby file too.
It's just a RTypedData which is Copy, so why not?
| @@ -0,0 +1,16 @@ | |||
| module Wasmtime | |||
| module TrapCode | |||
There was a problem hiding this comment.
Pulled this out of Trap and into its own module for symmetry with Wasmtime.
2 participants
Closes #24
This PR is (mainly) split in 2 commits:
TrapexceptionThe perfect solution for defining enums would've had the following properties:
case(e.g. not just methods)This is the closest I could come that ticked most of these boxes. The
current solution's downside is that if you add a trap code, you need
to go and update the Ruby file too.
I decided not to implement
Trap#traceand the frames stuff because (1) it's a net addition (2) I don't have a use case for it yet and (3) it isn't the most pressing thing.