Wasmtime GitHub notifications bot (Jun 29 2023 at 16:35): 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:
- 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.htmlPlease 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
wasmtimein a crate by adding suffixes to the symbols produced to assembly that currently producemultiple definitionerrors when attempting to use multiplewasmtimeversions.Starting this in draft to get feedback on the general approach before adding tests, tackling removing
linksfrom thefibercrate, and other refinements.
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)
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)
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.
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.
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 ## barto create the symbolfoobarso Wasmtime could invoke the C compiler with something like-DSYMBOL_APPEND=10_0_0or something like that.
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.
Wasmtime GitHub notifications bot (Jun 30 2023 at 00:16):adampetro updated PR #6673.
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::Customand alsoModuleVersionStrategy::None, and having this enabled by default would, from my understanding, break those two settings. Should it be opt-in, such as enabled with aWASMTIME_VERSIONED_EXPORTSenvironment variable to use the version-based suffix andWASMTIME_EXPORT_SUFFIXfor a custom suffix?
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
ModuleVersionStrategywhich 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.
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-fibercrate as well which would allow removing thelinksattribute there and enable multi-version linkage.
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-fibercrate as well which would allow removing thelinksattribute there and enable multi-version linkage.
Wasmtime GitHub notifications bot (Jun 30 2023 at 01:12):alexcrichton created PR review comment:
I think technically this
versioned_exportis only required on s390x?
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_trampolinemacro invocation)
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
Wasmtime GitHub notifications bot (Jun 30 2023 at 02:30):adampetro updated PR #6673.
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
ModuleVersionStrategywould 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.
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
Wasmtime GitHub notifications bot (Jun 30 2023 at 13:21):adampetro created PR review comment:
Got it, sounds good. Thanks!
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.
Wasmtime GitHub notifications bot (Jun 30 2023 at 22:26):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jun 30 2023 at 22:37):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 04 2023 at 20:05):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 04 2023 at 20:24):adampetro updated PR #6673.
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
weaklinkage which should enable this to exist with other definitions ok.
Wasmtime GitHub notifications bot (Jul 05 2023 at 14:51):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 05 2023 at 14:53):adampetro created PR review comment:
Makes sense, removed, thanks!
Wasmtime GitHub notifications bot (Jul 05 2023 at 14:59):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 05 2023 at 15:13):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 05 2023 at 15:29):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 05 2023 at 15:39):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 05 2023 at 20:10):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 05 2023 at 21:12):github-merge-queue[bot] updated PR #6673.
Wasmtime GitHub notifications bot (Jul 05 2023 at 21:12):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 18:06):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):adampetro has marked PR #6673 as ready for review.
Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):adampetro requested wasmtime-core-reviewers for a review on PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):adampetro requested itsrainy for a review on PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 20:41):adampetro requested wasmtime-default-reviewers for a review on PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 20:45):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 06 2023 at 21:41):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 21:45):alexcrichton has enabled auto merge for PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 23:12):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 23:32):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 06 2023 at 23:41):adampetro updated PR #6673.
Wasmtime GitHub notifications bot (Jul 07 2023 at 15:04):alexcrichton merged PR #6673.
Last updated: Dec 23 2024 at 13:07 UTC