Support multiple versions of wasmtime in the same crate

[adampetro]

issue #6660

This change enables support for including multiple versions of wasmtime in a crate by adding suffixes to the symbols produced to assembly that currently produce multiple definition errors when attempting to use multiple wasmtime versions.

Starting this in draft to get feedback on the general approach before adding tests, tackling removing links from the fiber crate, and other refinements.

[github-actions]

Subscribe to Label Action

cc @peterhuene

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 .github/subscribe-to-label.json configuration file.

Learn more.

[alexcrichton]

Thanks for working on this!

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)

[adampetro]

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.

[alexcrichton]

Nice! I like how this simplified things well. 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.

[adampetro]

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 e085825. I still need to handle the s390x case in a subsequent commit.

[alexcrichton]

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.

[adampetro]

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 ff81d02. 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?

[alexcrichton]

I sent adampetro#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.

[adampetro]

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?

[alexcrichton]

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 👍

[adampetro]

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)

[alexcrichton]

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.

[adampetro]

^ force push to rebase latest main and resolve merge conflict

[cfallin]

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.

[alexcrichton]

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"

[cfallin]

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?).

[alexcrichton]

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."

[adampetro]

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.

[alexcrichton]

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.

[bholley]

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

[adampetro]

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!

[mystor]

@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.

[alexcrichton]

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

[adampetro]

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

👍 I think it's ready. Thanks for all your help @alexcrichton!

[bholley]

@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.

[alexcrichton]

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