Alex Crichton (Jul 17 2020 at 15:49): Alex Crichton (Jul 17 2020 at 15:49):@fitzgen (he/him) filling out some Go bindings I found an issue where Func::new will panic if it's created with externef types and the Config doesn't have reference types enabled
Alex Crichton (Jul 17 2020 at 15:49):I'm a bit stuck thinking about this, though, as I'm not entirely sure how best to handle this
Alex Crichton (Jul 17 2020 at 15:49):the problem is that the trampoline generated by the wasmtime crate is attempting to use stack maps but cranelift's stack map support is not enabled so the register allocator naturally panics (as expected)
Alex Crichton (Jul 17 2020 at 15:50):I think we may want to avoid stack maps entirely for trampolines? I don't think they actually serve any purpose
fitzgen (he/him) (Jul 17 2020 at 16:01):Can we assert in Func::new et al that reference types are enabled in the config if they're being used?
fitzgen (he/him) (Jul 17 2020 at 16:01):that would at least be something we could document and have good assertion messages for
Alex Crichton (Jul 17 2020 at 16:02):hm so an assert isn't quite what would work for the bindings though
Alex Crichton (Jul 17 2020 at 16:02):wasm_func_new panicking brings down the process most of the time
Alex Crichton (Jul 17 2020 at 16:03):(that's what's happening right now)
fitzgen (he/him) (Jul 17 2020 at 16:03):I guess we would have to duplicate the check in a wasmtime_func_new :-/
Yury Delendik (Jul 17 2020 at 16:03):yay for wasmtime_ prefixed functions
fitzgen (he/him) (Jul 17 2020 at 16:04):I think you are technically correct that the trampolines don't need stack maps, but it is fairly subtle.
fitzgen (he/him) (Jul 17 2020 at 16:04):we could do some nasty r64 -> i64 hacks
Alex Crichton (Jul 17 2020 at 16:04):that's what I'm thinking yeah
Alex Crichton (Jul 17 2020 at 16:04):make it always work basically
fitzgen (he/him) (Jul 17 2020 at 16:05):definitely not worth it as a perf optimization, but to work around this issue, maybe
fitzgen (he/him) (Jul 17 2020 at 16:05):or alternatively
fitzgen (he/him) (Jul 17 2020 at 16:05):we can always enable reference types in the trampolines
fitzgen (he/him) (Jul 17 2020 at 16:05):that feels safer to me
Alex Crichton (Jul 17 2020 at 16:05):yeah that was my other thought
Alex Crichton (Jul 17 2020 at 16:05):I'll see if I can do that I think
fitzgen (he/him) (Jul 17 2020 at 16:05):cool, let me know how it goes
fitzgen (he/him) (Jul 17 2020 at 16:06):maybe we could have caught this if we extended the API call fuzzer
fitzgen (he/him) (Jul 17 2020 at 16:06):with reference types and instructions to create funcs
fitzgen (he/him) (Jul 17 2020 at 16:06):I wish the API call fuzzer was easier to extend, it is a little bit of a pain with all the scope stuff
Alex Crichton (Jul 17 2020 at 16:07):ok well that at least removes the panics
Alex Crichton (Jul 17 2020 at 16:07):seems fine for now!
Alex Crichton (Jul 17 2020 at 16:10):I have no idea if stack maps from trampolines even make their way into the Store
Alex Crichton (Jul 17 2020 at 16:10):I feel like our management of trampolines is very ad-hoc
Alex Crichton (Jul 17 2020 at 16:11):https://github.com/bytecodealliance/wasmtime/pull/2039
fitzgen (he/him) (Jul 17 2020 at 16:15):I think they will make their way into the store's stack map registry, I vaguely remember a common code path here
Last updated: Nov 22 2024 at 16:03 UTC