Stream: git-wasmtime

Topic: wasmtime / PR #6673 Support multiple versions of `wasmtim...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2023 at 16:35):

adampetro opened PR #6673 from adampetro:versioned-exports to bytecodealliance:main:

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

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

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2023 at 17:08):

alexcrichton submitted PR review:

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)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2023 at 17:08):

alexcrichton submitted PR review:

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)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2023 at 17:08):

alexcrichton created PR review comment:

If possible I think it'd be best to avoid the macro here and instead apply it on usages. For example when the impl_* symbols are defined they'd have #[versioned_fn], but otherwise this macro wouldn't be tampered with and the normal identifiers would be used everywhere.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2023 at 17:08):

alexcrichton created PR review comment:

Ideally this could always be enabled and handled internally, so could the goal for the end state of this PR be to remove this as an option?

If this is only an issue on s390x I think that's a targeted problem which can be fixed in isolation in the build script for the outlined assembly files.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2023 at 17:08):

alexcrichton created PR review comment:

One idea for handling this is to use preprocessor magic on s390x which supports I think foo ## bar to create the symbol foobar so Wasmtime could invoke the C compiler with something like -DSYMBOL_APPEND=10_0_0 or something like that.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2023 at 17:08):

alexcrichton created PR review comment:

This is one location where I think it'd be good to avoid this attribute because these are normal Rust functions whose symbols don't matter, so ideally versioning shouldn't come into play here.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 00:16):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 00:24):

adampetro created PR review comment:

I think it could makes sense not to use a feature for this. That being said, in discussion with @saulecabrera he mentioned that we likely want to support ModuleVersionStrategy::Custom and also ModuleVersionStrategy::None, and having this enabled by default would, from my understanding, break those two settings. Should it be opt-in, such as enabled with a WASMTIME_VERSIONED_EXPORTS environment variable to use the version-based suffix and WASMTIME_EXPORT_SUFFIX for a custom suffix?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 01:07):

alexcrichton created PR review comment:

Oh? In my mind this setting is purely about how we need some unmangled symbol names for things written in assembly and we need to be able to have "mangled" symbols per-wasmtime-version. That I would not expect to be related to the ModuleVersionStrategy which is only about what compiled artifacts look like, and as long as the right artifact is paired with the right wasmtime everything should work there as expected.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 01:12):

alexcrichton submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 01:12):

alexcrichton submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 01:12):

alexcrichton created PR review comment:

I think technically this versioned_export is only required on s390x?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 01:12):

alexcrichton created PR review comment:

I also think this may be causing the segfault on CI since the "mangled" symbols here should be the inline-assembly-generated trampolines, not the Rust-defined functions (e.g. the functions generated in the wasm_to_libcall_trampoline macro invocation)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 01:12):

alexcrichton created PR review comment:

I think this (and others on the top-level functions in this file) can be removed because these symbols aren't exported from the wasmtime library

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 02:30):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 11:24):

saulecabrera created PR review comment:

My line of thought here was mostly around verifying if the usage of ModuleVersionStrategy would be in conflict with versioning the symbols with the crate's version. But I don't necessarily have a particular use-case that demonstrates such conflict, I mostly wanted to make sure that we're all aligned that this is not a concern, which I think @alexcrichton has clarified in the previous comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 13:21):

adampetro created PR review comment:

Sounds good, I removed these and instead did the renaming inside of the wasmtime_asm_macros::asm_func! calls

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 13:21):

adampetro created PR review comment:

Got it, sounds good. Thanks!

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

alexcrichton created PR review comment:

Ah yeah I think we're all good then because the names of symbols in Wasmtime doesn't actually end up relating to compiled artifacts at all, so these end up being orthogonal knobs.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 22:26):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2023 at 22:37):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 04 2023 at 20:05):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 04 2023 at 20:24):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 14:13):

alexcrichton created PR review comment:

Oh this is actually the one symbol which needs to not be versioned since IIRC gdb sets a breakpoint on this or something like that. That's ok though due to the weak linkage which should enable this to exist with other definitions ok.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 14:51):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 14:53):

adampetro created PR review comment:

Makes sense, removed, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 14:59):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 15:13):

adampetro updated PR #6673.

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

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 15:39):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 20:10):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 21:12):

github-merge-queue[bot] updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 21:12):

adampetro updated PR #6673.

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

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):

adampetro has marked PR #6673 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):

adampetro requested wasmtime-core-reviewers for a review on PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):

adampetro requested itsrainy for a review on PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):

adampetro requested wasmtime-default-reviewers for a review on PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 06 2023 at 20:45):

alexcrichton submitted PR review.

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

adampetro updated PR #6673.

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

alexcrichton has enabled auto merge for PR #6673.

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

adampetro updated PR #6673.

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

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 06 2023 at 23:41):

adampetro updated PR #6673.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 07 2023 at 15:04):

alexcrichton merged PR #6673.

Last updated: Nov 22 2024 at 16:03 UTC