interrupt triggering SIGILL #74
Comments
|
Ok thanks! I tried this again and let it run in the background and indeed got the fault eventually. I mostly try to investigate these sorts of issues by tracking them down in a debugger if I can, and afterwards just thinking hard about them. (unfortunately these are often pretty gnarly to investigate) That being said after thinking about this and seeing it in a debugger I think I know the issue of what's going on here. The problem is that wasmtime needs to initialize trap handlers per-thread and while it's doing that for the first thread the Go runtime is presumably migrating the main goroutine to a different OS thread and wasmtime isn't re-initializing (currently that's an unsafe operation in Rust and requires a manual opt-in to initialize it). I'll look into fixing this in Wasmtime in the near future! |
Platforms Wasmtime supports may have per-thread initialization that needs to run before WebAssembly. For example Unix needs to setup a sigaltstack and macOS needs to set up mach ports. In bytecodealliance#2757 this per-thread setup was moved out of the invocation of a wasm function, relying on the lack of Send for Store to initialize the thread at Store creation time and never worry about it later. This conflicted with [wasmtime's desired multithreading story](bytecodealliance#2812) so a new [`Store::notify_switched_thread` was added](bytecodealliance#2822) to explicitly indicate a Store has moved to another thread (if it unsafely did so). It turns out though that it's not always easy to determine when a `Store` moves to a new thread. For example the Go bindings for Wasmtime are generally unaware when a goroutine switches OS threads. This led to bytecodealliance/wasmtime-go#74 where a SIGILL was left uncaught, making it appear that traps aren't working properly. This commit revisits the decision in bytecodealliance#2757 and moves per-thread initialization back into the path of calling into WebAssembly. This is differently from before, though, where there's still only one TLS access on the path of calling into WebAssembly, unlike before where it was a separate access. This allows us to get the speed benefits of bytecodealliance#2757 as well as the flexibility benefits of not having to explicitly move a store between threads. With this new ability this commit deletes the recently added `Store::notify_switched_thread` method since it's no longer necessary.
|
I've verified that bytecodealliance/wasmtime#2863 fixes the issue on my end at least |
|
Thanks a lot, both for fixing this so quickly and providing insights. 😃 |
* Bring back per-thread lazy initialization Platforms Wasmtime supports may have per-thread initialization that needs to run before WebAssembly. For example Unix needs to setup a sigaltstack and macOS needs to set up mach ports. In #2757 this per-thread setup was moved out of the invocation of a wasm function, relying on the lack of Send for Store to initialize the thread at Store creation time and never worry about it later. This conflicted with [wasmtime's desired multithreading story](#2812) so a new [`Store::notify_switched_thread` was added](#2822) to explicitly indicate a Store has moved to another thread (if it unsafely did so). It turns out though that it's not always easy to determine when a `Store` moves to a new thread. For example the Go bindings for Wasmtime are generally unaware when a goroutine switches OS threads. This led to bytecodealliance/wasmtime-go#74 where a SIGILL was left uncaught, making it appear that traps aren't working properly. This commit revisits the decision in #2757 and moves per-thread initialization back into the path of calling into WebAssembly. This is differently from before, though, where there's still only one TLS access on the path of calling into WebAssembly, unlike before where it was a separate access. This allows us to get the speed benefits of #2757 as well as the flexibility benefits of not having to explicitly move a store between threads. With this new ability this commit deletes the recently added `Store::notify_switched_thread` method since it's no longer necessary. * Fix a test compiling
|
Ok with bytecodealliance/wasmtime#2863 merged I'm gonna close this and this'll get into the packages on the next wasmtime version bump which should be soon-ish |
|
Heya. Thanks again for fixing this! Are there any plans for a release including this? 😄 |
|
Ah I was waiting on a wasmtime version for this but I realized I can publish 0.26.1, so I've pushed that up. |
|
Thank you! It looks great on my end. Thanks again 👏 |
* Bring back per-thread lazy initialization Platforms Wasmtime supports may have per-thread initialization that needs to run before WebAssembly. For example Unix needs to setup a sigaltstack and macOS needs to set up mach ports. In bytecodealliance#2757 this per-thread setup was moved out of the invocation of a wasm function, relying on the lack of Send for Store to initialize the thread at Store creation time and never worry about it later. This conflicted with [wasmtime's desired multithreading story](bytecodealliance#2812) so a new [`Store::notify_switched_thread` was added](bytecodealliance#2822) to explicitly indicate a Store has moved to another thread (if it unsafely did so). It turns out though that it's not always easy to determine when a `Store` moves to a new thread. For example the Go bindings for Wasmtime are generally unaware when a goroutine switches OS threads. This led to bytecodealliance/wasmtime-go#74 where a SIGILL was left uncaught, making it appear that traps aren't working properly. This commit revisits the decision in bytecodealliance#2757 and moves per-thread initialization back into the path of calling into WebAssembly. This is differently from before, though, where there's still only one TLS access on the path of calling into WebAssembly, unlike before where it was a separate access. This allows us to get the speed benefits of bytecodealliance#2757 as well as the flexibility benefits of not having to explicitly move a store between threads. With this new ability this commit deletes the recently added `Store::notify_switched_thread` method since it's no longer necessary. * Fix a test compiling
👉 main.go, go.sum and go.mod are here
Running
go run main.go, iteration 388 gives meSome environment information,
I'll gladly supply more information, or do some guided poking -- I'm afraid I don't really know where to start. 😅
Follow-up to #60.
The text was updated successfully, but these errors were encountered: