Stream: git-wasmtime

Topic: wasmtime / PR #7167 fix(wasmtime-cli): wrong linker in wa...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 08:36):

eduardomourar edited PR #7167:

This will fix the error one would get while trying to run the wasmtime CLI with -S http because the async functions were being added to the linker. Additionally, we will allow shadowing in linker when using WASI HTTP in order to prevent the following error:

import of wasi:clocks/wall-clock defined twice

Also, to prevent it from breaking in the future, a simple handcrafted test has been added to the CLI test suite.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 08:36):

eduardomourar has marked PR #7167 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 08:36):

eduardomourar requested wasmtime-core-reviewers for a review on PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 08:36):

eduardomourar requested alexcrichton for a review on PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 12:33):

eduardomourar updated PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:18):

eduardomourar updated PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:46):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:46):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:46):

alexcrichton created PR review comment:

Thanks for adding this! This is unfortunately not really possible to change though and without the source used to generate this we can't feasibly change this over time. I think the best solution is to enable the cli-tests suite here to use programs from the test-programs crate. That's a larger refactoring though and probably best done in a separate PR, so it's ok to skip a test for this PR specifically and rely on local testing.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:46):

alexcrichton created PR review comment:

I think it would be best if allow_shadowing was disabled because that's otherwise easy to accidentally run into issues.

There's unfortunately not a great solution for the error you're running into here, so in the meantime I think it would be best to expose individual add_to_linker functions per-interface and only invoke the wasi-http-related ones here.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:51):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:51):

eduardomourar created PR review comment:

i actually built the component out of the original core module wasi-http (which was handcrafted before in order to make it more maintainable).

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:54):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:54):

eduardomourar created PR review comment:

i agree it will be very hard to keep it up-to-date while we are still iterating over WASI. maybe we can keep the original core module and build a component on the fly with wasm-tools?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:57):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:57):

eduardomourar created PR review comment:

alright, i can do that

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:04):

eduardomourar updated PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:08):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:08):

eduardomourar created PR review comment:

done. as it is only temporary, i exposed function only in sync module and kept out of docs.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:21):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:21):

alexcrichton created PR review comment:

Yes that makes sense but it's not clear to me, for example, how to regenerate this test. This means that if there's an update it would be quite difficult to update this. Even the core module will change because the wasi-http interfaces might change.

The "real fix" here is to use source code rather than compiled wat or wasm, but that would require further refactorings to provide the test-programs programs to the WASI CLI tests here. I think it's best to have no tests until that's done, and I don't think it would be too large of a refactoring so I can work on it after this PR lands.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:38):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:38):

eduardomourar created PR review comment:

there is no real source code because it was done by hand. it is not hard to make it, though. i just wanted to keep it very simple without a server-side in the cli as it takes longer to run.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:42):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:42):

alexcrichton created PR review comment:

Yes that's all fine, but again I would personally prefer not to check in a large *.wat file which we don't know how to regenerate and change. I don't doubt that you've generated this from source, but there's no instructions in-tree about what the generation process is or what the source is. I think it's best to leave this out, manually test this PR, and I'll follow up with a better testing strategy to re-land this.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 14:57):

eduardomourar updated PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:03):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:03):

eduardomourar created PR review comment:

just added a test to test-programs called outbound_request_response_build that does not require a server. i just don't know how to find the path to reference in the wasmtime cli tests file.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:12):

eduardomourar updated PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:21):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:21):

alexcrichton created PR review comment:

Hm ok so in my opinion this PR can't land with this *.wat test. This is even if it's generated from the test you've added in-tree. This is local knowledge on this PR's comment which will be lost basically as soon as the PR lands, meaning that if you or I aren't the next person to touch this file, which is quite likely, then they'll not know what to do. To solve this dilemma one of a few things need to happen:

Personally I prefer the first option because, as you pointed out, we'll want to backport this. In that sense I'd rather not block this PR. I'm happy to defer to you though on which route you'd prefer.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:29):

eduardomourar updated PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:30):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:30):

eduardomourar created PR review comment:

in think option 1 is the best one too.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:32):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:32):

eduardomourar created PR review comment:

i have removed the wat file and ignored that particular test case. let me know and i can help re-enabling the test back when ready

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 15:35):

eduardomourar edited PR #7167:

This will fix the error one would get while trying to run the wasmtime CLI with -S http because the async functions were being added to the linker. Additionally, we will expose only the necessary functions to allow shadowing in linker when using WASI HTTP in order to prevent the following error:

import of wasi:clocks/wall-clock defined twice

Also, to prevent it from breaking in the future, a simple handcrafted test has been added to the CLI test suite.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:39):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:39):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:39):

alexcrichton created PR review comment:

For the future this isn't necessary as println! should work in guests. If it doesn't that's a bug we should document and fix.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:49):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:49):

eduardomourar created PR review comment:

If I use println, it gets translated to wasi preview1 but I wanted to exercise the preview2 functions

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:56):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:56):

alexcrichton created PR review comment:

Ah that's sort of right but also not quite accurate. It's correct that println! uses the Rust standard library which uses wasi preview1, but that's adapted to preview2 with the adapter, meaning that println! ends up calling the same preview2 function this one calls.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:58):

eduardomourar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 16:58):

eduardomourar created PR review comment:

But not in the core module, right?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 17:24):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 17:24):

alexcrichton created PR review comment:

Sorry I'm not exactly sure what you mean, but when using println! the core module produced by rustc will refer to wasi_snapshot_preview1::fd_write if that's what you mean. That core import is then hooked up to the adapter, and the adatper imports preview2 instead.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 17:26):

alexcrichton merged PR #7167.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2023 at 00:55):

alexcrichton created PR review comment:

To follow up here, https://github.com/bytecodealliance/wasmtime/pull/7182 is the refactoring I had in mind and this is the un-ignore of the test

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2023 at 00:55):

alexcrichton submitted PR review.

Last updated: Nov 22 2024 at 17:03 UTC