This repository was archived by the owner on Aug 31, 2023. It is now read-only.
Refactor the validator for function references#5
Merged
CosineP merged 3 commits intoeffect-handlers:func-reffrom Jul 28, 2022
Merged
Refactor the validator for function references#5CosineP merged 3 commits intoeffect-handlers:func-reffrom
CosineP merged 3 commits intoeffect-handlers:func-reffrom
Conversation
This patch refactors the validator to align with the structure of the validation algorithm in the appendix of the WebAssembly Specification 2.0 + function references document.
CosineP
approved these changes
Jul 25, 2022
Collaborator
CosineP
left a comment
There was a problem hiding this comment.
This is awesome! We may have a handful of merge conflicts because I just went and fixed the tests, but it's clear that Bot did indeed make everything much cleaner!
Comment on lines
262
to
+263
| /// If `Some(T)` is returned then `T` was popped from the operand stack and | ||
| /// matches `expected`. If `None` is returned then it means that `None` was | ||
| /// matches `expected`. If `Bot` is returned then it means that `None` was |
Collaborator
There was a problem hiding this comment.
If T is returned
Also: should we pass expected: Bot instead of having it be optional?
| let actual = match expected { | ||
| None => actual_ty, | ||
| Some(expected_ty) => { | ||
| if !resources.matches(actual_ty, expected_ty) { |
Collaborator
There was a problem hiding this comment.
Great. I realized today that <= is equivalent to == for all types besides those added by function references, so this is correct all the time
Author
There was a problem hiding this comment.
Yep, the matches relation conservatively extends the notion of equality over types. Conservative means here: expressions that were true or false before matches remain true & false after the introduction of matches, i.e. the relation only affects the truth value of the new terms.
Comment on lines
+643
to
+649
| if !resources.matches(actual, expected) { | ||
| bail_op_err!( | ||
| "type mismatch: expected {}, found {}", | ||
| ty_to_str(expected), | ||
| ty_to_str(actual) | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
This is certainly redundant, right?
Author
There was a problem hiding this comment.
Possibly! I will have a look at it again.
Comment on lines
+872
to
+882
| fn is_num_or_vec(ty: ValType) -> bool { | ||
| matches!( | ||
| ty, | ||
| ValType::I32 | ||
| | ValType::I64 | ||
| | ValType::F32 | ||
| | ValType::F64 | ||
| | ValType::V128 | ||
| | ValType::Bot | ||
| ) | ||
| } |
Collaborator
There was a problem hiding this comment.
Good catch, this will merge conflict but that's fine. As I'm sure you saw, the 2.0 spec has this definition, but not function-references, which is why we accidentally reverted this
Author
There was a problem hiding this comment.
Yep! Should be a simple conflict to resolve, though.
Comment on lines
+2188
to
+2192
| // if !self.features.tail_call { | ||
| // return Err(OperatorValidatorError::new( | ||
| // "tail calls support is not enabled", | ||
| // )); | ||
| // } |
Collaborator
There was a problem hiding this comment.
Presumably we still want this?
Author
There was a problem hiding this comment.
I think so, but the testsuite failed when I added it in, so we might need to tweak something somewhere...
| Operator::RefAsNonNull => { | ||
| self.check_function_references_enabled()?; | ||
| if let Some(RefType { heap_type, .. }) = self.pop_ref(resources)? { | ||
| self.check_heap_type(heap_type, resources)?; |
Collaborator
There was a problem hiding this comment.
Ah, I see, push_operand should take care of this
Author
There was a problem hiding this comment.
Yep! The validation is much more uniform/simpler since Bot is part of the type structure, and not external as it were with the Option representation.
Comment on lines
-2219
to
+2222
| let (ty, kind) = self.jump(relative_depth)?; | ||
| let non_null = if let Some(RefType { heap_type, .. }) = self.pop_ref(resources)? { | ||
| self.check_heap_type(heap_type, resources)?; | ||
| ValType::Ref(RefType { | ||
| nullable: false, | ||
| heap_type, | ||
| }) | ||
| } else { | ||
| // TODO: i'm confused. arbitrary but still tested as being | ||
| // a ref? | ||
| ValType::Ref(RefType { | ||
| nullable: false, | ||
| heap_type: HeapType::Func, | ||
| }) | ||
| }; | ||
| // validates that t* matches block type by popping each t and | ||
| // pushing them again. TODO: This is not quite right with | ||
| // subtyping, and has to be changed everywhere | ||
| for ty in label_types(ty, resources, kind)?.rev() { | ||
| let rt = self.pop_ref(resources)?; |
Collaborator
There was a problem hiding this comment.
Ah great, all fixed! Much better, thanks!
I had some kind of point in that comment about how pop then push no longer preserves the types under subtyping, but the pseudocode in the spec uses it and it clearly passes the tests, so I must be wrong. I might have to think about it to convince myself.
Comment on lines
-2253
to
+2259
| // ref ht <= tl | ||
| // tl = ref ht | tl = ref null ht | ||
| Some(ValType::Ref(RefType { heap_type, .. })) => { | ||
| self.check_heap_type(heap_type, resources)?; | ||
| // pop a nullable variant ie both nullable and | ||
| // non-nullable references are allowed here | ||
| self.pop_operand( | ||
| Some(ValType::Ref(RefType { | ||
| heap_type, | ||
| nullable: true, | ||
| })), | ||
| resources, | ||
| )?; | ||
| Some(rt1 @ ValType::Ref(_)) => { | ||
| if !resources.matches(rt0, rt1) { | ||
| bail_op_err!( | ||
| "type mismatch: expected {} but found {}", | ||
| ty_to_str(rt0), | ||
| ty_to_str(rt1) | ||
| ) | ||
| } |
dhil
commented
Jul 28, 2022
Co-authored-by: Daniel Hillerström <daniel.hillerstrom@ed.ac.uk>
CosineP
added a commit
that referenced
this pull request
Jul 28, 2022
Co-authored-by: Daniel Hillerström <daniel.hillerstrom@ed.ac.uk> Co-authored-by: cosine <CosineP@users.noreply.github.com>
dhil
added a commit
that referenced
this pull request
Aug 5, 2022
Co-authored-by: Daniel Hillerström <daniel.hillerstrom@ed.ac.uk> Co-authored-by: cosine <CosineP@users.noreply.github.com>
dhil
added a commit
that referenced
this pull request
Feb 15, 2023
* [WIP] Parse reference types from binary This identifies reference types' magic numbers and advances the parser. It parses heap types and assembles them into a type for non-nullable references. But, it does not actually change the syntax yet so it just panics. It remains to be implemented for nullable references and the sugars. * Add nullable reference types and desugar forms Untested * [WIP] Factor RefType out of ValType This checks for wasmparser, but no tests and no other crates yet * [func-refs] Fix parser tests, finish ref type parsing * [func-refs] Support ref types in pretty-printer * [func-refs] Fix pretty-printer bugs Missing parenthesis and printing a valtype instead of a heap type for func.ref * [func-refs] Add support for validator func.ref * [func-refs] Add syntax in encoder but not support * [func-refs] Fix printer giving bad types of indices * Clean up debugging nonsense * [func-refs] Implement subtyping * Implement a binary reader for function references instructions. (#1) * Change ref.null to take heaptype, not valtype * Update error message to match tests * Hack around error message test * [WIP] [func-refs] Add call_ref static semantics WIP because untested * Static semantics for function references (#2) Further changes to validation need to be made, at least including adding subtyping to more places. * [func-refs] Add subtyping to elements * Subtyping relation (#4) * Refactor the validator for function references (#5) Co-authored-by: Daniel Hillerström <daniel.hillerstrom@ed.ac.uk> Co-authored-by: cosine <CosineP@users.noreply.github.com> * [func-refs] Assume matches V128 V128 = true It looks to me that the function references spec was forked from webassembly before vectors were merged, so it has no definitive answer on V128 subtyping. It also means we accidentally reverted `select` to an outdated spec. I think for now it's reasonable to use common sense for V128 subtyping, and at some point the proposal is presumably planned to be rebased? * [func-refs] Support validate flag, br_table.wast notes * [func-refs] Bypass inconsistent test message This is a mismatch from the original spec tests, however in function-references the same exact test for some reason has a slightly different message, so the bypass needs another case. * [func-refs] Require defaultable table types This is from the specification of defaultability from here: WebAssembly/function-references#62 That change still doesn't include locals, which are under discussion. The implementation as of now still requires defaultable locals as in the test suite. * [func-refs] Add subtyping to table.copy * [func-refs] Correct f type structural equivalence This checks that arguments to a function are themselves structurally equivalent. Note that this is at least as difficult as a recursive subtyping relation would be, since we now need to implement vt1 <= vt2 AND vt1 == vt2 including for function types, despite the fact that <= is (even stated to be) preferable in this case * [func-refs] Perform some cleanup Move some things into order and fix some doc comments. This is not enough cleanup, more needs to be done * Fix bad brace matching from github commit * [func-refs] Order operator validation as in spec * [func-refs] Restore feature check in return_call_ref * Fix todos in validator/core.rs (#8) * Add missing brace * Fix bad merge * Fix failing test case (#11) * Implement missing functions in EmptyResources trait in func.rs * Fix failing test * [func-refs] Reorder wasmprinter instructions * [func-refs] Remove incorrect comment; format * Merge with upstream; simplify the implementation of matches. * Run cargo fmt * Fix compile errors with the rest of the workspace * Leave `unimplemented!()` for `Ref` types other than funcref/externref * Leave `unreachable!()` for `Bot` cases * Print old `funcref` and `externref` for those types Helps improve compatibility with other text parsers and additionally fixes a few tests which have "golden output" and assert that `funcref` and `extenref` are printed. * Fix a doctest example * Fix indirect_call subtype; small comments * Fix wasmparser benchmark * Fixup merge conflicts * Add type payload to `call_ref` and `return_call_ref` * Fix a typo * Fix tests * Remove ValType::Bot * Rename `HeapType::Index` to `HeapType::TypedFunc`. * Enable new working, disable new broken tests * Pack ValType into 4 bytes * [func-refs] Implement initialization checking * Fix printing of element kind * Update dump * wip * wip * Fix failing `*.wast` tests and align APIs This commit fixes some decoding of types in the `wasmparser` crate and then additionally adds support in `wasmparser`, `wasm-encoder`, etc, for the initialization expression of a table being specified. This involved aligning the type hierarchy of `wasm-encoder` with that of `wasmparser` which involved quite a few changes in a number of crates. Overall though this is mostly syntactic changes without much meat happening here. * Touch up some docs and style * Handle some minor TODO comments * Improve an error message * Update test exemptions * Remove redundant branches * Attempt to fix fuzz/fuzz_targets/validate.rs * fmt * Remove `HeapType::Bot` from the public API Move it as an internal implementation detail of the validator. --------- Co-authored-by: Luna Phipps-Costin <phipps-costin.l@northeastern.edu> Co-authored-by: cosine <CosineP@users.noreply.github.com> Co-authored-by: Alex Crichton <alex@alexcrichton.com> Co-authored-by: cosine <trash@cosine.online>
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
This patch refactors the validator to align with the structure of the
validation algorithm in the appendix of the WebAssembly Specification
2.0 + function references document.