Stream: git-wasmtime

Topic: wasmtime / PR #9196 Use the CallHook::CallingHost and Ret...

Wasmtime GitHub notifications bot (Sep 03 2024 at 15:49):

elliottt opened PR #9196 from elliottt:trevor/component-host-call-hooks to bytecodealliance:main:

We never implemented calling the CallingHost and ReturningFromHost hooks
for component host calls.

co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 03 2024 at 17:37):

alexcrichton submitted PR review:

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?

Wasmtime GitHub notifications bot (Sep 03 2024 at 17:37):

alexcrichton created PR review comment:

Could you expand the catch_unwind_and_longjmp to encompass the invocation of the call hooks too?

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:12):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:12):

elliottt has marked PR #9196 as ready for review.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:13):

elliottt requested alexcrichton for a review on PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:13):

elliottt requested wasmtime-core-reviewers for a review on PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:14):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:15):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:16):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:16):

elliottt created PR review comment:

This differs from the core wasm behavior, in that trapping in f does not register the return from host or return from wasm hooks. @alexcrichton, is this reasonable, or is there something missing from this PR?

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:20):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:20):

elliottt created PR review comment:

I think I've answered my own question: we shouldn't be using and_then to guard the exiting hook.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:23):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:24):

elliottt edited PR #9196:

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>

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 04 2024 at 17:44):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 18:31):

alexcrichton submitted PR review:

Nice tests :+1:

I think this will be good to go with additional tests for using the realloc and post-return canonical options since I suspect those aren't covered by call hooks just yet

Wasmtime GitHub notifications bot (Sep 04 2024 at 18:31):

alexcrichton created PR review comment:

I think this is the same as call_wrapped_async_func?

Wasmtime GitHub notifications bot (Sep 04 2024 at 18:31):

alexcrichton created PR review comment:

To avoid needing to call this twice could the above use store.as_context_mut() as an argument? That should effectively reborrow to allow store to be used here again

Wasmtime GitHub notifications bot (Sep 04 2024 at 18:31):

alexcrichton created PR review comment:

Could this perhaps be use'd from the other call hook test?

Wasmtime GitHub notifications bot (Sep 04 2024 at 18:31):

alexcrichton created PR review comment:

Could this be call_hook(...)?? (notably using ? instead of if let)

Wasmtime GitHub notifications bot (Sep 04 2024 at 18:35):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 19:52):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 19:56):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 20:11):

alexcrichton created PR review comment:

Could you shift this to being a result of a list? As-is this doesn't require a realloc, but as a return value it'll require a realloc

Wasmtime GitHub notifications bot (Sep 04 2024 at 20:11):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2024 at 20:11):

alexcrichton created PR review comment:

Instead of defining this here you can also use the "realloc" export of the $libc module

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:08):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:11):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:50):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:51):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:51):

elliottt created PR review comment:

Here are the assertions about post-return, you can see that the wasm calls increment after the .post_return() call.

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:52):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:53):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:53):

elliottt created PR review 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.

Wasmtime GitHub notifications bot (Sep 04 2024 at 23:54):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 05 2024 at 18:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 05 2024 at 18:06):

alexcrichton created PR review 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)

Wasmtime GitHub notifications bot (Sep 05 2024 at 18:30):

elliottt updated PR #9196.

Wasmtime GitHub notifications bot (Sep 05 2024 at 18:32):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 05 2024 at 18:32):

elliottt created PR review 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.

Wasmtime GitHub notifications bot (Sep 05 2024 at 19:04):

elliottt merged PR #9196.

Last updated: Nov 22 2024 at 17:03 UTC