-
Notifications
You must be signed in to change notification settings - Fork 333
feat: (CM64) Validate 64bit waitables
#2514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
michael-weigelt
wants to merge
1
commit into
bytecodealliance:main
Choose a base branch
from
michael-weigelt:mwe/waitable
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+12
−6
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to best do this, except just checking if either signature matches. I don't seem to have enough context to know the memory type here.
Any ideas, @alexcrichton ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! This one's... tricky.
Two possible solutions I can think of:
context.{get,set}does, but in retrospect I'm not a huge fan of this. This makes the core wasm significantly different and bindings generators have to have more#[cfg](or similar) to have different names on different targets. It's all doable, but there's also not a ton of need for this.The general north star here is WebAssembly/component-model#378 which points to solution (1), however. In the end that may be the best thing to do here because then if an intrinsic is provided by a host runtime there's a single canonical name for it, and runtimes with 64-bit support would have to provide twice the intrinsics as a 32-bit-only runtime.
cc @lukewagner on this point -- the high-level question here is what would a hypothetical
BuildTargets.mdname be for intrinsics likewaitable-set.waitw.r.t. 32-bit and 64-bit. The type signature is naturally i32 or i64 depending on the linear memory, but the question here is the convention used for "what does the guest import". Taking 378 literally we'd double all of the intrinsics for 64-bit linear memories, but to confirm this would require that runtimes which "flatten" a component to provide twice the set of intrinsics by default -- everything for 32-bit and for 64-bit. That feels not amazing but also pretty clear what's expected of runtimes as well, fundamentally it is indeed twice the possible imports. Wanted to run this by you and make sure this didn't set off any bells in your headThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! Indeed CM/#378 is already thinking in the "different import names" direction by having a
cm32p2prefix (which I guess becomescm32p3in this context) for all names which then naturally leads tocm64p3, hence twice the possible imports. For the Guest C ABI, I suppose none of this forces the host one way or the other, but, depending on how ba/rfcs/#46 plays out, the "lower-component" tool might have the same question and want to use the same names in some cases, and in that context, I suppose a plain Core WebAssembly host would appreciate each name having a single fixed type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, in that case @michael-weigelt let's go ahead and do something similar to what the context get/set intrinsics did and basically use different names for intrinsics of different types. One option is to use
$rootvs$root64vs maybe a new$root32, but I don't think the name really matters too too much, just whatever's easiest to implement is ok. The true bikeshed will be in 378 with "official" names for all of these intrinsics, and I don't think we're at risk of clashing.