Stream: git-wasmtime

Topic: wasmtime / issue #6673 Support multiple versions of `wasm...

Wasmtime GitHub notifications bot (Jun 29 2023 at 16:45):

github-actions[bot] commented on issue #6673:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jun 30 2023 at 00:18):

adampetro commented on issue #6673:

One suggestion I might have though is that instead of changing the literal identifier used in Rust code to one with a version appended I think this would be a good use for #[export_name = "foo"] or #[link_name = "foo"] (depending on context). That way the symbol in Rust wouldn't change, but only the symbol at the binary level (which is what we care about the most here)

This is a really great suggestion, thank you! I did not know this was possible, but it simplifies things considerably.

Wasmtime GitHub notifications bot (Jun 30 2023 at 13:24):

adampetro commented on issue #6673:

If you're up for it as well it'd be awesome to include this dependency in the wasmtime-fiber crate as well which would allow removing the links attribute there and enable multi-version linkage.

I started tackling this in https://github.com/bytecodealliance/wasmtime/pull/6673/commits/e085825c6984762919c47d25025086f761841ffa. I still need to handle the s390x case in a subsequent commit.

Wasmtime GitHub notifications bot (Jun 30 2023 at 15:24):

alexcrichton commented on issue #6673:

This all looks great to me, thanks again! If you're up for handling s390x that'd be much appreciated, but if not then I think as long as it's not broken it's ok to land this and that's ok too.

Wasmtime GitHub notifications bot (Jun 30 2023 at 22:36):

adampetro commented on issue #6673:

This all looks great to me, thanks again! If you're up for handling s390x that'd be much appreciated, but if not then I think as long as it's not broken it's ok to land this and that's ok too.

Thanks for all of your help reviewing! I attempted handling s390x in https://github.com/bytecodealliance/wasmtime/pull/6673/commits/ff81d020954aa1347f6189d18ca478c9789b8bf6. I tried to pass the suffix with the -DSYMBOL_APPEND=10_0_0 approach as you suggested but found that it didn't process it for .S files so I went with a manual preprocess reading the file, doing a string replace, and outputting to a new file in OUT_DIR. I'm definitely open to feedback if there is a better approach, maybe there's a way to define the assembly from a .c file to get the compiler to do the preprocessing? And I assume somewhere in CI it will test with s390x?

Wasmtime GitHub notifications bot (Jul 01 2023 at 17:21):

alexcrichton commented on issue #6673:

I sent https://github.com/adampetro/wasmtime/pull/1 to avoid the need to copy files around, IIRC I dealt with voodoo like that a long time ago, but I'm not sure that's the simplest it can be.

This reminded me though that there's one more location at crates/runtime/src/helpers.c which will need to be versioned as well, and I think it's the same as the *.S files in theory? Other than that though I think this is good to go! And yeah s390x will be tested on a full CI on merge.

Wasmtime GitHub notifications bot (Jul 04 2023 at 20:37):

adampetro commented on issue #6673:

Thanks! I added versioned symbols for crates/runtime/src/helpers.c in the most recent commit.

Would I also need to do something similar in crates/fiber/src/windows.c?

Wasmtime GitHub notifications bot (Jul 05 2023 at 14:12):

alexcrichton commented on issue #6673:

Ah yes I forgot about that! I think though that's the last one and this should be good to go once that's updated :+1:

Wasmtime GitHub notifications bot (Jul 05 2023 at 15:06):

adampetro commented on issue #6673:

I'll add the new crate to scripts/publish.rs. Should it also be added to supply-chain/config.toml? (I just searched where wasmtime-asm-macros was referenced in the repo as I assumed it would be similar for wasmtime-versioned-export-macros)

Wasmtime GitHub notifications bot (Jul 05 2023 at 15:09):

alexcrichton commented on issue #6673:

Yeah I think it's fine to keep the same strategy as wasmtime-asm-macros and do basically the same thing in all the various places.

Wasmtime GitHub notifications bot (Jul 05 2023 at 15:42):

adampetro commented on issue #6673:

^ force push to rebase latest main and resolve merge conflict

Wasmtime GitHub notifications bot (Jul 05 2023 at 15:47):

cfallin commented on issue #6673:

Drive-by clarifying thought that I just worked out: I was contemplating the non-composability of POSIX signals (as one does...) and wondering how multiple runtimes in one process would fare if each is trying to handle traps from its own respective instances. It turns out that Wasmtime chains to previous signal handlers if it wasn't active (TLS slot was null).

I think this mostly works, except I haven't been able to convince myself yet that it's safe if a Wasm instance in wasmtime-1 calls to host code which then calls a Wasm instance in wasmtime-2 which traps. Then both TLS slots are active; if wasmtime-1 catches the signal (registered its handlers last), will it properly chain to wasmtime-2? If so, we should probably document somewhere how this interaction is expected to work.

Wasmtime GitHub notifications bot (Jul 05 2023 at 17:23):

alexcrichton commented on issue #6673:

Good questions! Ones I had forgotten to consider with this...

I think though we should be good in theory. If you've got wasmtime-1 and wasmtime-2 both on the stack then if one faults and the others' handler ends up running first the handler should end up determining that the signal should not be caught and defer to the next handler. This is more-or-less falling out of the "we don't catch signals not caused by wasm" and the signals from wasmtime-1 appear like non-wasm signals to wasmtime-2 because the faulting address won't be in any known region.

I do agree though it'd be good to document this! It'd be better yet to even test it, although that part is sort of hard. As for where to document this I'm not entirely sure other than perhaps in the code along the lines of "this should be written to work when multiple wasmtimes are in the same process"

Wasmtime GitHub notifications bot (Jul 05 2023 at 18:09):

cfallin commented on issue #6673:

It'd be better yet to even test it, although that part is sort of hard

In theory we could have a test (maybe an integration test to avoid overhead in the usual cargo test case) that depends on some arbitrary previous version of wasmtime, as well as the current in-repo one.

Or: if we wrapped some globals into a context struct (at least the signal handlers and the allocated tls-slot) we could do something like

let ctx1 = wasmtime::ProcessContext::new();
let ctx2 = wasmtime::ProcessContext::new();
let engine1 = wasmtime::Engine::new_with_context(&ctx1);
let engine2 = wasmtime::Engine::new_with_context(&ctx2);
// ... test with instances calling each other via host code

and Engine::new() uses some default ProcessContext that's a lazy-static global or somesuch. I'm not sure how difficult it would be to plumb the context to all the places it's needed though -- e.g. the signal handlers themselves need some root to start from (another TLS slot that is global?).

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:13):

adampetro commented on issue #6673:

In theory we _could_ have a test (maybe an integration test to avoid overhead in the usual cargo test case) that depends on some arbitrary previous version of wasmtime, as well as the current in-repo one.

I tried out this approach in https://github.com/bytecodealliance/wasmtime/pull/6673/commits/6312b7062660d45372b4724b31ede2c518a6471b and it is relatively simple and passes. Is that what you had in mind? If not, I can revert that commit. I understand if you would rather not bring in an arbitrary previous version of wasmtime.

Apologies for the force push, I had to rebase due to another merge conflict with main.

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:16):

cfallin commented on issue #6673:

That makes sense to me at least; I'll defer to @alexcrichton whether the impact on build-time is reasonable or not. Just curious -- would you be able to time the CI run (./ci/run-tests.sh locally, after removing target/) before and after pulling in the second build of Wasmtime?

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:35):

adampetro commented on issue #6673:

Just curious -- would you be able to time the CI run (./ci/run-tests.sh locally, after removing target/) before and after pulling in the second build of Wasmtime?

For sure! On M1 Pro MacBook. Before:

./ci/run-tests.sh  1280.97s user 124.35s system 417% cpu 5:37.00 total

After:

./ci/run-tests.sh  1320.65s user 131.57s system 418% cpu 5:46.61 total

I stopped rust-analyzer and ran cargo clean before each invocation.

Wasmtime GitHub notifications bot (Jul 05 2023 at 20:51):

alexcrichton commented on issue #6673:

I personally think that adding a previous copy as a dependency is going to become pretty difficult to maintain over time and isn't worth the benefit yet. One of the issues with this is that if we ever update a dependency then we'll get cargo deny errors about duplicate dependencies since the old version of wasmtime will still have the old dependency -- this can sort of already be seen with cargo deny failing in CI on this PR. This is probably fixable, but I feel like that would be somewhat the tip of the iceberg in this case.

Additionally Chris's suggestion would be a good way to go but I dont think we can do that because it'd use the same copy of wasmtime-runtime-the-compiled-rlib which means that everything is using one TLS slot. While this shouldn't cause issues in theory it does mean that it's not a "perfect" emulation of multiple versions and may accidentally miss something crucial.

FWIW we have had integration issues along the lines of signal handlers in the past, for example with breakpoint on macOS and not wanting to use mach exception ports but instead use unix signals. Now that I type this out I think a multi-version wasmtime on macOS may not work because there's two threads handling exceptions and it's a race as to which gets a message and there's no delegate-to-the-next handler ability like there is with signals (or at least not implemented I think, I'm not expert on mach)

So it may perhaps be best to instead fix issues on an issue-by-issue basis instead of declaring "wasmtime multi-version works great". My guess is there's weird platform intricacies here and whatever we test we'll never suss them all out. In that sense @adampetro's embedding might be the best test case for this feature as opposed to testing it in the repository itself?

Wasmtime GitHub notifications bot (Jul 05 2023 at 21:00):

adampetro commented on issue #6673:

Sounds good, I'll remove that commit.

In that sense @adampetro's embedding might be the best test case for this feature as opposed to testing it in the repository itself?

FWIW, at least originally our plan is to use multi-version for compilation, while still executing with a single version. This will enable us to have a full cache of compiled modules when cutting over to a new version for execution.

Wasmtime GitHub notifications bot (Jul 05 2023 at 21:04):

adampetro commented on issue #6673:

Unrelated question, what's the best way to deal with cargo vet for a new unreleased crate? cargo vet --locked appears to pass when I omit the crate from supply-chain/config.toml; is that what I should do?

Wasmtime GitHub notifications bot (Jul 05 2023 at 21:12):

alexcrichton commented on issue #6673:

Oh interesting! Oddly enough none of this is necessary technically if all you're doing is compiling modules since these are all "runtime bits" as opposed to compiler bits. That being said while Wasmtime has a compiler-less build mode it doesn't have a runtime-less build mode so there's no way to build wasmtime without the runtime bits right now.

To assuage @cfallin's concerns then one possiblity might be to have a runtime assertion that only one wasm runtime is loaded in to the process. I'm not sure if that's possible to do, though. I think that would work for your embedding @adampetro though?

Otherwise though for cargo vet I think you'll need to run cargo vet locally which should make some changes to supply-chain/imports.lock (I think) which can be committed here.

Wasmtime GitHub notifications bot (Jul 05 2023 at 21:22):

adampetro commented on issue #6673:

To assuage @cfallin's concerns then one possiblity might be to have a runtime assertion that only one wasm runtime is loaded in to the process. I'm not sure if that's possible to do, though. I think that would work for your embedding @adampetro though?

Yes, only being able to execute modules with one version would work for my use-case

Otherwise though for cargo vet I think you'll need to run cargo vet locally which should make some changes to supply-chain/imports.lock (I think) which can be committed here.

When I remove mention of the new crate from supply-chain/config.toml, cargo vet succeeds but doesn't make any changes to supply-chain/imports.lock. Or if I leave it in supply-chain/config.toml but with audit-as-crates-io = false then it also passes. But with audit-as-crates-io = true like for other similar crates from the workspace it fails.

Wasmtime GitHub notifications bot (Jul 05 2023 at 22:52):

alexcrichton commented on issue #6673:

Ah sorry I'm new to this as well so I'm trying to remember what's going on here. I think the issue may be that supply-chain/audits.toml needs an entry that looks like:

[[wildcard-audits.wasmtime]]
who = "Bobby Holley <bobbyholley@gmail.com>"
criteria = "safe-to-deploy"
user-id = 73222 # wasmtime-publish
start = "2021-10-29"
end = "2024-06-26"
notes = "The Bytecode Alliance is the author of this crate."

(to copy an existing entry)

This is a "trusted file" though which we typically audit carefully in PRs and contributors typically don't modify. For example when existing crates are updated we as project authors will send a PR with audits which a contributor then rebases on top of to get cargo vet passing. That being said I can't add the audit myself easily because the crate doesn't exist yet (chicken-and-egg oh dear).

In lieu of that can you add this entry?

[[wildcard-audits.wasmtime-versioned-export-macros]]
who = "Alex Crichton <alex@alexcrichton.com>"
criteria = "safe-to-deploy"
user-id = 1 # Alex Crichton (alexcrichton)
start = "2023-07-05"
end = "2024-07-05"
notes = "The Bytecode Alliance is the author of this crate."

Wasmtime GitHub notifications bot (Jul 06 2023 at 15:20):

adampetro commented on issue #6673:

Even with the suggested supply-chain/audits.toml entry, having audit-as-crates-io = true in supply-chain/config.toml fails:

ERROR   × Cannot fetch crate information, 'wasmtime-versioned-export-macros' does
  │ not exist.

With audit-as-crates-io = false, it succeeds regardless of the supply-chain/audits.toml entry.

Wasmtime GitHub notifications bot (Jul 06 2023 at 17:36):

alexcrichton commented on issue #6673:

Hm ok sorry for the runaround here! I think in this situation let's set audit-as-crates-io to false and we can fix it up later as necessary.

@bholley you might be interested in some of the above, but I think this boils down to we're adding a new crate to this repository and I think it may not be playing well with audit-as-crates-io since that seems like it may require the crate to exist on crates.io? (I may be missing something as well of course). I think it worked for all our other crates since they were already published, but as a new crate this may be running into something new.

Wasmtime GitHub notifications bot (Jul 06 2023 at 18:08):

bholley commented on issue #6673:

@mystor can you figure out what's going on here?

Wasmtime GitHub notifications bot (Jul 06 2023 at 18:10):

adampetro commented on issue #6673:

Hm ok sorry for the runaround here! I think in this situation let's set audit-as-crates-io to false and we can fix it up later as necessary.

No worries, sounds good and thanks for the help!

Wasmtime GitHub notifications bot (Jul 06 2023 at 18:29):

mystor commented on issue #6673:

@bholley you might be interested in some of the above, but I think this boils down to we're adding a new crate to this repository and I think it may not be playing well with audit-as-crates-io since that seems like it may require the crate to exist on crates.io? (I may be missing something as well of course). I think it worked for all our other crates since they were already published, but as a new crate this may be running into something new.

Yes, it is required that all crates marked as audit-as-crates-io = true be published on crates.io. We only support audits for published crates and published crate versions (if you have a newer version in tree, we require an audit for the most recently published version), so if there's no version of the crate on crates.io, you cannot have the crate marked as audit-as-crates-io = true, as there is no version which it is possible to have an audit for.

Once there is a published version of the crate you can switch to audit-as-crates-io = true to check that you're publishing audits for that version.

Wasmtime GitHub notifications bot (Jul 06 2023 at 19:11):

alexcrichton commented on issue #6673:

Ok thanks for the clarification!

In that case @adampetro this all looks good to me, so happy to merge when you feel it's ready

Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):

adampetro commented on issue #6673:

Ok thanks for the clarification!

+1, thanks!

In that case @adampetro this all looks good to me, so happy to merge when you feel it's ready

:+1: I think it's ready. Thanks for all your help @alexcrichton!

Wasmtime GitHub notifications bot (Jul 10 2023 at 21:37):

bholley commented on issue #6673:

@alexcrichton If this crate isn't published, it shouldn't need an audit-as-crates-io entry at all. That field is specifically for crates which appear first-party but whose name and metadata match a published crate.

Wasmtime GitHub notifications bot (Jul 10 2023 at 21:47):

alexcrichton commented on issue #6673:

True! This will be published soon though, it just hasn't been included in any release prior yet.

Last updated: Nov 22 2024 at 17:03 UTC