Use the CallHook::CallingHost and ReturningFromHost with components #9196
Conversation
We never implemented calling the CallingHost and ReturningFromHost hooks for component host calls. co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
There was a problem hiding this comment.
Thanks! Mind also adding some tests for this? There's some previous call-hook-related tests which might be good to copy/augment.
Additionally can you double-check that invoking the malloc function calls the hook as well? And also that we call the hooks on entry to wasm?
| let res = store | ||
| .0 | ||
| .call_hook(CallHook::CallingHost) | ||
| .and_then(|_| crate::runtime::vm::catch_unwind_and_longjmp(|| func(instance, types, store))) |
There was a problem hiding this comment.
Could you expand the catch_unwind_and_longjmp to encompass the invocation of the call hooks too?
| // Before post-return, there will only have been one call into wasm. | ||
| assert_eq!(store.data().calls_into_wasm, 1); | ||
| assert_eq!(store.data().returns_from_wasm, 1); | ||
|
|
||
| export.post_return(&mut store)?; | ||
|
|
||
| // There are no host calls in this example, but the post-return does increment the count of | ||
| // wasm calls by 1, putting the total number of wasm calls at 2. | ||
| let s = store.into_data(); | ||
| assert_eq!(s.calls_into_host, 0); | ||
| assert_eq!(s.returns_from_host, 0); | ||
| assert_eq!(s.calls_into_wasm, 2); | ||
| assert_eq!(s.returns_from_wasm, 2); |
There was a problem hiding this comment.
Here are the assertions about post-return, you can see that the wasm calls increment after the .post_return() call.
| let message = String::from("hello, world!"); | ||
| let res = export.call(&mut store, (message.as_bytes(),))?.0; | ||
| let result = res.to_str(&store)?; | ||
| assert_eq!(&message, &result); | ||
|
|
||
| export.post_return(&mut store)?; | ||
|
|
||
| // There is one host call for the host-side realloc, and then two wasm calls for both the | ||
| // `list8-to-str` call and the guest realloc call for the list argument. | ||
| let s = store.into_data(); | ||
| assert_eq!(s.calls_into_host, 1); | ||
| assert_eq!(s.returns_from_host, 1); | ||
| assert_eq!(s.calls_into_wasm, 2); | ||
| assert_eq!(s.returns_from_wasm, 2); |
There was a problem hiding this comment.
Here's the handling of the allocated result, and the assertions about the number of host calls encountered. There is only one host call counted, as only realloc is used. However, there are two wasm calls for the realloc of the list<u8> argument, and the list8-to-str call itself.
| @@ -248,7 +248,11 @@ impl<'a, T> LowerContext<'a, T> { | |||
| ) -> Result<usize> { | |||
| let realloc_func_ty = Arc::clone(unsafe { (*self.instance).realloc_func_ty() }); | |||
| let realloc_func_ty = realloc_func_ty.downcast_ref::<FuncType>().unwrap(); | |||
| self.options | |||
|
|
|||
| self.store.0.call_hook(CallHook::CallingHost)?; | |||
There was a problem hiding this comment.
Here and below I think this is CallingWasm and ReturningFromWasm?
Also would it be possible to push this further down into the realloc call below? Or, I'm not sure if this works, even further into TypedFunc::unchecked_call? (or w/e API this bottoms out to)
There was a problem hiding this comment.
It turns out this is already counted as a wasm call, and the host call hooks were just unnecessary and incorrect. I had an incorrect assumption about what the Options::realloc function was doing, and assumed it was handling host-side reallocs. Given that those aren't a thing and we're already accounting for wasm hooks, the changes to this module were unnecessary.
…ytecodealliance#9196) * Use the CallHook::CallingHost and ReturningFromHost with components We never implemented calling the CallingHost and ReturningFromHost hooks for component host calls. co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> * Add tests * Run hooks under `catch_unwind_and_longjmp` * Cleanup the imports * Remove todo * Ensure that returning hooks are run * Appease clippy * Suggestions from code review * Reuse infrastructure from the core-wasm call-hook test * Remove redundant test * Add a realloc test * Switch the realloc test to handle returning a string * Rework the realloc test to check that we're tracking host reallocs * Use the call hook in the realloc host call * Unnecessary pub mod * Add a post-return test * Remove unnecessary assertions * Format * Remove incorrect hook calls for realloc uses --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
* Use the CallHook::CallingHost and ReturningFromHost with components (#9196) * Use the CallHook::CallingHost and ReturningFromHost with components We never implemented calling the CallingHost and ReturningFromHost hooks for component host calls. co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> * Add tests * Run hooks under `catch_unwind_and_longjmp` * Cleanup the imports * Remove todo * Ensure that returning hooks are run * Appease clippy * Suggestions from code review * Reuse infrastructure from the core-wasm call-hook test * Remove redundant test * Add a realloc test * Switch the realloc test to handle returning a string * Rework the realloc test to check that we're tracking host reallocs * Use the call hook in the realloc host call * Unnecessary pub mod * Add a post-return test * Remove unnecessary assertions * Format * Remove incorrect hook calls for realloc uses --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> * Add a RELEASES.md entry for 9196 --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
We never implemented calling the CallingHost and ReturningFromHost hooks for component host calls. This PR supports those hooks, and adds some tests.
co-authored-by: Nick Fitzgerald fitzgen@gmail.com