Skip to content

feat: (CM64) Validate 64bit waitables#2514

Draft
michael-weigelt wants to merge 1 commit intobytecodealliance:mainfrom
michael-weigelt:mwe/waitable
Draft

feat: (CM64) Validate 64bit waitables#2514
michael-weigelt wants to merge 1 commit intobytecodealliance:mainfrom
michael-weigelt:mwe/waitable

Conversation

@michael-weigelt
Copy link
Copy Markdown
Contributor

No description provided.

}

if let Some(info) = names.waitable_set_wait(name) {
// [ValType::I32, Addrtype]
Copy link
Copy Markdown
Contributor Author

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 ?

Copy link
Copy Markdown
Member

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:

  1. Have different names for 32-bit and 64-bit intrinsics. This is similar to what 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.
  2. Defer validation of signatures until we've determined whether this module is 32-bit or 64-bit. This feels appealing to me because then guest languages only need to import one name and everything generally works out. It would make this code more complicated, however.

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.md name be for intrinsics like waitable-set.wait w.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 head

Copy link
Copy Markdown

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 cm32p2 prefix (which I guess becomes cm32p3 in this context) for all names which then naturally leads to cm64p3, 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.

Copy link
Copy Markdown
Member

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 $root vs $root64 vs 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants