Wasmtime GitHub notifications bot (Jul 21 2020 at 22:45): Wasmtime GitHub notifications bot (Jul 21 2020 at 22:45):alexcrichton commented on Issue #2059:
I've marked this as a draft while the wasmparser changes have yet to be released.
Wasmtime GitHub notifications bot (Jul 21 2020 at 23:00):github-actions[bot] commented on Issue #2059:
Subscribe to Label Action
cc @bnjbvr, @peterhuene
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "lightbeam", "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 (Jul 22 2020 at 16:24):alexcrichton commented on Issue #2059:
Ok the wasmparser bits are published and this should be good to go!
Wasmtime GitHub notifications bot (Jul 22 2020 at 17:09):github-actions[bot] commented on Issue #2059:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
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 (Jul 24 2020 at 12:59):bnjbvr commented on Issue #2059:
The bits to implement don't seem terribly hard, we already required most of this information anyways, so it was around. It's nice to have a proper API to make sense of it!
One request, though: in wasmparser, most of the trait functions return a
&Self::Type(or&Self::FuncType, etc.), which makes it a bit hard to implement in some places. For instance, our Type representation is a plain packed u32 at the moment: the ret type being a borrow makes it hard to return a plain primitive value. Could it be aCow<Self::Type>instead? (Unless there's a better API for this of course; my Rust knowledge is reaching its boundaries here.)Because of the above issue, I haven't gone through implementing it entirely in Spidermonkey. Since I wouldn't want to block this from landing, would it make sense to allow both APIs right now, mark the former one as deprecated, and allow for implementing the new one for clients who want it?
Wasmtime GitHub notifications bot (Jul 24 2020 at 13:30):bnjbvr commented on Issue #2059:
(I'm being told Rust nightly has generic associated types, which would allow putting a lifetime type parameter in the
Typetrait declaration, making it possible to return something that depends on&'selfor not; this is not a viable possibility at the moment, though.)
Wasmtime GitHub notifications bot (Jul 28 2020 at 17:07):alexcrichton commented on Issue #2059:
After some discussion on Zulip, I think https://github.com/bytecodealliance/wasm-tools/pull/68 will help address the issues here, so I'll get that landed/published and update it here before we merge this.
Wasmtime GitHub notifications bot (Jul 31 2020 at 15:34):alexcrichton commented on Issue #2059:
Ok I think
wasmparsershould have the necessary changes now, @bnjbvr did you want to double-check this again?
Wasmtime GitHub notifications bot (Jul 31 2020 at 16:24):bnjbvr commented on Issue #2059:
As agreed on Zulip, since i won't be around for some time, this should land later to give Julian/Chris an opportunity to look into Spidermonkey changes this coming week. Thanks Alex!
Wasmtime GitHub notifications bot (Aug 26 2020 at 18:56):alexcrichton commented on Issue #2059:
Ok I've rebased this and further updated wasmparser to 0.62.0, which pulled in some multi-memory and memory64 bits. Multi-memory may work as-is (except for
memory.copybetween memories) but memory64 is just left asunimplemented!(). In any case we'll want more official support for this later, for now it's just getting things to compile.
Wasmtime GitHub notifications bot (Aug 27 2020 at 09:42):bnjbvr commented on Issue #2059:
Thanks for the update; i'll look into using this for Spidermonkey, probably early next week.
Wasmtime GitHub notifications bot (Sep 15 2020 at 22:50):cfallin commented on Issue #2059:
I've done some work on the SpiderMonkey embedding of Cranelift to adapt to this change; we making progress. I rebased your patch here: commit
Patch on SM side for reference/curiosity (this is based on @bnjbvr's excellent work): on bug 1655042
There are a bunch of error-message regexes in unit tests that need to be updated, but we're almost there. Thanks for the patience!
Wasmtime GitHub notifications bot (Sep 15 2020 at 22:50):cfallin edited a comment on Issue #2059:
I've done some work on the SpiderMonkey embedding of Cranelift to adapt to this change; we're making progress. I rebased your patch here: commit
Patch on SM side for reference/curiosity (this is based on @bnjbvr's excellent work): on bug 1655042
There are a bunch of error-message regexes in unit tests that need to be updated, but we're almost there. Thanks for the patience!
Wasmtime GitHub notifications bot (Sep 16 2020 at 00:50):alexcrichton commented on Issue #2059:
Thanks for the update! I've gone ahead and pushed a rebased version here as well if that helps too.
Wasmtime GitHub notifications bot (Sep 30 2020 at 19:55):alexcrichton commented on Issue #2059:
I've fixed the recent merge conflicts with a rebase!
Wasmtime GitHub notifications bot (Sep 30 2020 at 20:32):cfallin commented on Issue #2059:
Getting closer on my end too; FWIW I had to make one small change to fix some tests, exposing a junk-bytes-at-end condition as an error return rather than panic: commit (feel free to incorporate). I just have to tackle some table/reftypes-related test failures and then SpiderMonkey will be ready for this!
Wasmtime GitHub notifications bot (Sep 30 2020 at 21:18):alexcrichton commented on Issue #2059:
Oh oops, good catch! The intention was to let wasmparser's validator catch that naturally, so I pushed up a slightly tweaked commit which should remove the panic as well.
Wasmtime GitHub notifications bot (Oct 04 2020 at 05:08):cfallin commented on Issue #2059:
I've gotten all tests to pass in SpiderMonkey using this validator, now! In order to do so, I locally updated the
wasmparserdep incranelift-wasmto 0.63.0; this included an update to the bytecode-operator enum so I have a local commit here on top of your two commits. (That patch-stack is rebased to currentmainas well.) I'm not sure if the update is strictly necessary, now, but I did so as I was working through some weird validation mismatches, and have now baked the new version's error messages into the unit-test updates, so it has become a requirement for that reason :-)Given that, I think there isn't any further reason to hold this back -- thanks for waiting on SM to catch up!
(A tangential note, as hinted above -- the SpiderMonkey test suite has a whole bunch of "this wasm module should fail to compile in a way that matches this regex" tests, so we should be careful updating any error messages going forward, or at least consider the cost; I wish there were a better way (standardized error messages as per a spec?) but this is what we have for now...)
Wasmtime GitHub notifications bot (Oct 04 2020 at 20:01):fitzgen commented on Issue #2059:
(A tangential note, as hinted above -- the SpiderMonkey test suite has a whole bunch of "this wasm module should fail to compile in a way that matches this regex" tests, so we should be careful updating any error messages going forward, or at least consider the cost; I wish there were a better way (standardized error messages as per a spec?) but this is what we have for now...)
The spec tests do have expected error messages and, except for a few exceptions, we match them in
wasmparser. Does SpiderMonkey also (newly?) do this? Or am I misunderstanding which tests you're talking about?
Wasmtime GitHub notifications bot (Oct 04 2020 at 22:33):cfallin commented on Issue #2059:
Ah, I hadn't realized there were standard error messages. I can dig further into why SpiderMonkey doesn't hit issues with this (perhaps the harness is tweaked?) but actually all of the issues I hit were in the SpiderMonkey-specific test suite. An example here matches on a specific "type mismatch: expression has type A but expected B" message;
wasmparser(version 0.63) emits "type mismatch: expected Some(A), found Some(B)" and A and B are e.g.I32,FuncRefrather thani32,funcref. (Seems this is just a directDebugrendering of the Rust type representation.) So I had to tweak a bunch of/a/regexes to/(a)|(b)/(because SM still has the old backends). Not ideal, but workable for now.
Wasmtime GitHub notifications bot (Oct 05 2020 at 16:01):alexcrichton commented on Issue #2059:
"standard" is a bit of a stretch in that they're not super descriptitive, it's typically "type mismatch" for everything related to a type error and the implementation typically provides more descriptive stuff afterwards. That being said of
Some(...)is showing up in wasmparser parser errors that's probably a bug on our end where we were too lazy to stringify something and went withformat!("{:?}")for convenience. We'd be more than happy to update the error messages in wasmparser to be more readable, I don't think they've received too much love previously!It's great to hear though that SpiderMonkey is coming along well so I'm going to carry over the approval from before and go ahead and merge this. Thanks again for the heads up @cfallin!
Last updated: Dec 23 2024 at 12:05 UTC