wasmtime / Issue #2118 Enable ref types and bulk memory o... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / Issue #2118 Enable ref types and bulk memory o...

Wasmtime GitHub notifications bot (Aug 07 2020 at 19:22):

The only prior experience we have for this is multi-value, and we didn't actually enable that by default until it was merged into the upstream spec itself. In that sense this is much earlier to enable-by-default than our precedent.

Given that, I wonder if we might want to take this moment to discuss/agree about when to enable new features by default? I personally feel that merging into the spec is probably too slow, and stage 4 (which these proposals are at) feels like it's the right time. It's what Firefox did!

If others agree, could this also add some documentation in the book to when we turn a feature on by default? I would say the requirements for a Wasmtime wasm feature to be on by default would be:

Upstream proposal in stage 4

Complete implementation in Wasmtime

No major bugs or design work needs to be done

Fuzzing enabled for awhile (week or so?)

In terms of the content of this PR, r=me, just wanted to point out the procedural point!

Wasmtime GitHub notifications bot (Aug 07 2020 at 19:29):

alexcrichton commented on Issue #2118:

Actually some other requirements I would add are:

Implemented in the wasmtime crate's API

Implemented in the C API

Implemented in at least one language binding (e.g. Go, Python, .NET)

Wasmtime GitHub notifications bot (Aug 07 2020 at 19:32):

fitzgen commented on Issue #2118:

This is a good point to bring up, thanks Alex. Once we find consensus on what is required here, we should probably document it in our contributing guide.

You proposed requirements sound good to me, but I would even expand the last point about fuzzing to include a review of the ways that we've exposed the feature to the fuzzers.

Is it just enabled in the "compile" fuzz target's config or did we write custom fuzz targets to explicitly exercise running (rather than just compiling) this feature?

Did we add seeds to our corpora that use the feature?

Generally, how confident are we that the fuzzers have actually exercised this stuff? (e.g. does introducing a panic on purpose to some corner case get caught by the fuzzer pretty quickly?)

In particular, if we aren't pretty confident about that last point, I don't think we are ready to turn the feature on by default yet.

According to these standards, I feel good about reference types, and okay about bulk memory. I think bulk memory could probably use more runtime testing, rather than just "can we compile modules that use it" testing. For reference types, we have specialized fuzz targets that actually run the modules, not just compile them.

Wasmtime GitHub notifications bot (Aug 07 2020 at 19:48):

alexcrichton commented on Issue #2118:

Oh yeah and to be clear I also feel good about the implementation we have of reference types and bulk memory myself.

Also, as an aside, would you be up (after this lands) to send a similar PR to wasm-tools to update Default for WasmFeatures in wasmparser?

Wasmtime GitHub notifications bot (Aug 07 2020 at 19:50):

github-actions[bot] commented on Issue #2118:

Subscribe to Label Action

cc @bnjbvr, @peterhuene

<details>
This issue or pull request has been labeled: "cranelift", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

bnjbvr: cranelift

peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 07 2020 at 20:44):

alexcrichton commented on Issue #2118:

If you want to add some docs to https://bytecodealliance.github.io/wasmtime/stability.html (or that section, maybe not exactly that page), that might be a good place to add some info?

Wasmtime GitHub notifications bot (Aug 07 2020 at 23:59):

fitzgen commented on Issue #2118:

@alexcrichton okay I added two new bits of docs:

A table, similar to https://webassembly.org/roadmap/, documenting our wasm proposal support. This is intended to be mostly outward facing.

A couple checklists for implementing and enabling-by-default new wasm proposals. These are intended to be developer/reviewer focused.

Want to take another look?

Wasmtime GitHub notifications bot (Aug 08 2020 at 00:16):

alexcrichton commented on Issue #2118:

Thanks for writing all that up!

Last updated: Oct 23 2024 at 20:03 UTC