Skip to content

Conversation

@pchickey
Copy link
Contributor

@pchickey pchickey commented Apr 29, 2020

Prior to this patch, a witx file describes a collection of interfaces that a module can import. However, we don't have a way to describe completely what a module must export, or which of many available interfaces are available for import.

This PR introduces the concept of a profile, which is a set of available imports (specified as witx modules) and a set of required exports (specified as witx @interface funcs).

In Wasi snapshot 1, we only have one module exposed for import, and the concept of an Command just tacks on one additional requirement, the export of a "_start" function with no params and no results. So, this profile is easy to write and pretty much trivial.

However, in ephemeral, we have a significant number of Wasi modules, and it seems like the Executable concept should be extended to mean that all of them are available.

Additionally, we have a new concept of a Reactor, which has a different required function "_initialize" and then a set of required functions available. Nascent specs like proxy-wasm (cc @PiotrSikora) can now specify what the required set of export functions are using a profile. And, vendors like Fastly can now specify a witx profile that includes both Wasi and the additional interfaces they provide.

The grammar for profiles is:

(profile $profilename 
   (expose $modulename)*
   (require (@interface func (export "exportname") ...))*
)

One thing that profiles (and witx more generally) cannot yet solve is making interfaces from different snapshots of Wasi available in the same profile. Unfortunately, because typenames exist in a single global scope and their names are re-used (not necessarily in an equivalent manner) between snapshots, we can't use multiple snapshots of Wasi into the same witx document. At Fastly we are working to provide a profile that supports every snapshot of Wasi simultaneously (using polyfills to implement old snapshots in terms of the latest one) so we will need to resolve this problem in a future PR.

@pchickey pchickey requested a review from sunfishcode April 29, 2020 19:23
@programmerjake
Copy link
Contributor

Sounds like a good idea, however, I'd argue for not requiring all those modules for executables -- there are use cases where some of them should be left out, such as reproducible builds not needing networking support.

@pchickey
Copy link
Contributor Author

For cases where not all modules are available, I think that either the vendor or (if useful to many parties) the standard should specify a separate profile for a given use case.

@sbc100
Copy link
Member

sbc100 commented Apr 29, 2020

Generally I like this idea.

When would these profiles be used in practice? Would a program self declare somewhere that it conforms to the profile of wasi_snapshot_preview1_executable? Would a program declare somehow that it require a certain profile be provided by the embedder? Is it not already possible to know that module depends on simply by looking at its imports?

It this designed for answering questions like "will my module run on fastly?" without actually trying to run it there? Like a a kind of conformance checking?

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only looked at the witx parts so far, but it looks like a good start! I expect profiles will be a key building block as we add more APIs.

Should wasi_ephemeral_profile.witx be named wasi_ephemeral_command.witx, to fit the pattern of files being named after the main thing they define? No strong opinion here, but it seems if we add more profiles, we'll need a naming convention that has the profile name in the filename.

;;; WASI modules available for import, and exporting a start function.
(profile $wasi_ephemeral_executable
;;; Executables must export a start function:
(@interface func (export "_start"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the rest of witx (and wit), "export" means something that the API exports, and "import" means something that the API expects you to export so that the API can import it from you. Here, "_start" is something the API expects you to export, so it seems like "import" would be a better way to describe this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is inconsistent. Part of my motivation was to re-use the existing InterfaceFunc parser and AST. I have been thinking as profile as a predicate a module can satisfy, and from that perspective I think the way export and import are used make sense, but I can be convinced otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independently of import vs. export, could you move "_start" into its own regular witx module and (expose ...) it here? We'll likely need that as soon as we go to define another command-like profile. And I think it would also happen to sidestep this whole question of import vs. export in a profile, because then "_start" would just be an import in a witx file similar to "memory", and the profile would just contain exposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we use (require (@interface func (export "name")...)) - that will correspond to (expose (module...))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also leaves us open to (optional (@interface func ...)) which I could see being useful in a reactor context like proxy-wasms

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 7722981

(import $wasi_ephemeral_proc)
(import $wasi_ephemeral_random)
(import $wasi_ephemeral_sched)
(import $wasi_ephemeral_sock)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With module imports on their way, this syntax has the risk of being confusing. Would it make sense to use a different keyword instead of import here, like (include ...) or (import ...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(expose ...) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expose works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 7722981

@pchickey
Copy link
Contributor Author

pchickey commented Apr 29, 2020

@sbc100 Yes, having a description that can answer "will my module run on Fastly" as easily as possible is my primary motivation with this work. That description wouldn't be part of the module itself, its just something you can ask about a module. witx doesn't handle parsing a Wasm module to see if it conforms to a spec, but there's no particular reason it couldn't - we just need to see if all of the module's imports are covered by the available imports, and all exports in the profile are provided. I want profiles to be useful beyond just Fastly, of course, and discussions we had about proxy-wasm and reactors is a big motivator for it as well.

@sbc100
Copy link
Member

sbc100 commented Apr 29, 2020

@sbc100 Yes, having a description that can answer "will my module run on Fastly" as easily as possible is my primary motivation with this work. That description wouldn't be part of the module itself, its just something you can ask about a module. witx doesn't handle parsing a Wasm module to see if it conforms to a spec, but there's no particular reason it couldn't - we just need to see if all of the module's imports are covered by the available imports, and all exports in the profile are provided. I want profiles to be useful beyond just Fastly, of course, and discussions we had about proxy-wasm and reactors is a big motivator for it as well.

But how do you see them being used in practice though? Outside of this repo where will they appear in the world? For example with these things get named inside of wasm modules somehow? I think I maybe be missing something here.

@pchickey
Copy link
Contributor Author

pchickey commented Apr 29, 2020

But how do you see them being used in practice though? Outside of this repo where will they appear in the world? For example with these things get named inside of wasm modules somehow? I think I maybe be missing something here.

We plan to publish a witx document containing the Fastly profile, the Wasi types and modules we support, plus all of the other types and modules we support. (This is also, incidentally, why the new render subcommand in the witx example code is useful - it takes multiple witx files, resolves their use statements, and renders it into a single file). Given that document, you can use any profile in it as a predicate that a module can satisfy.

Our CLI tool will automate checking a wasm module emitted by various toolchains (clang, rustc, assemblyscript, go...) against the correct profile. Each toolchain has various ways that you can specify an import function (directly, or in some transitive dep) that can't be provided by the Fastly engine, so checking the emitted wasm module is the way to go.

@pchickey
Copy link
Contributor Author

Per feedback from @sunfishcode I have updated the code (and PR description) to use (expose $modulename) to specify an interface available for import, and (require (@interface func (export "funcname")...)) to specify a function required to be exported.

pchickey added 2 commits May 1, 2020 16:14
validation narrows it down to one export.

also, fix some stuff around where doc comments are permitted/dropped -
previously we would have just erased a doc comment on `(noreturn)`.
@pchickey
Copy link
Contributor Author

pchickey commented May 2, 2020

@sunfishcode and I discussed this further today, and we changed the syntax for required exports to (import "name" (@interface func ...)). This meant changing export names in (@interface func ...) to be optional. Unfortunately, because the AST is a bit sloppy around how names work, the round-tripping test currently does not work correctly. I still need to figure out how to change the way interface funcs are represented in the AST, hopefully finding a way to minimize breaking existing code.

I'll be on vacation May 4-8, so I won't be back to work on this until May 11.

@pchickey
Copy link
Contributor Author

pchickey commented Dec 2, 2020

I never got back to this. If I do in the future I'll open a new PR.

@pchickey pchickey closed this Dec 2, 2020
@pchickey pchickey deleted the pch/witx_profile branch December 2, 2020 21:26
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.

5 participants