GC: wasmparser: Subtypes #1059

imikushin · 2023-06-09T22:20:42Z

Introducing the SubType struct:

pub struct SubType {
    pub is_final: bool,
    pub supertype_idxs: Vec<u32>,
    pub structural_type: StructuralType,
}

Here, StructuralType is an enum (previously named Type).

supertype_idxs is a vector of indexes of supertypes, limited to at most one supertype as per GC MVP.

A valid subtype matches its supertypes. Supertypes cannot be final.

Introducing the `SubType` struct: ```rust pub struct SubType { pub is_final: bool, pub supertype_idxs: Vec<u32>, pub structural_type: StructuralType, } ``` Here, `StructuralType` is an enum (previously named `Type`). `supertype_idxs` is a vector of indexes of supertypes, limited to at most one supertype as per GC MVP. A valid subtype matches its supertypes. Supertypes cannot be final.

imikushin · 2023-06-14T18:00:56Z

x86_64-linux and x86_64-windows CI jobs were canceled for some reason.

fitzgen

Thanks!

This is looking good, but I have a few nitpicks and also a request for more complete testing.

Once those things are addressed we can merge this.

crates/wasm-encoder/src/core/types.rs

crates/wasmparser/src/readers/core/types.rs

crates/wasmparser/src/validator/component.rs

crates/wasmparser/src/validator/core.rs

crates/wasmparser/src/validator/types.rs

tests/local/gc/gc-subtypes.wat

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

Expanded test coverage for different kinds of subtypes in gc-subtypes.wast and gc-subtypes-invalid.wast. Validation of mutability, nullability, alignment of subtypes and supertypes, and the equality of expanded param/result types to those of the parent are included.

fitzgen

LGTM with a couple minor nitpicks. I'll be on vacation for a week starting tomorrow, so maybe @alexcrichton can merge this once you've addressed the nitpicks.

Thanks!

crates/wasm-encoder/src/core/types.rs

crates/wit-component/src/gc.rs

tests/local/gc/gc-subtypes.wat

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

This reverts commit 3765018.

…es-invalid.wast`

imikushin · 2023-06-30T19:49:02Z

@alexcrichton I believe I've addressed all @fitzgen's feedback (thank you!).

Currently, field namespace is global (per module). But if a field of the same name is at the same position, that should be okay.

crates/wast/src/names.rs

crates/wasmparser/src/validator/core.rs

alexcrichton · 2023-07-03T15:09:58Z

crates/wasmparser/src/readers/core/types.rs

+pub(crate) trait Matches {
+    fn matches<'a, F>(&self, other: &Self, type_at: &F) -> Result<bool>
+    where
+        F: Fn(u32) -> Result<&'a SubType>;


One thing I might recommend is to remove the Result here since indices at this point should always be valid because the definitions of types are validated to all be correct. Additionally it might be better to pass a concrete type here rather than a generic Fn(u32) -> &SubType but instead have something like cx: &Validator or similar.

One thing I might recommend is to remove the Result here since indices at this point should always be valid because the definitions of types are validated to all be correct.

Actually, the primary use case for matches() is to validate if the type definitions are correct.

I don't think that's the case? Whenever a type definition is reached as part of validation there are no type-matching queries performed at that time and instead the structure of the defined type is validated. This means that the indices are all valid for the type section after it's read, so in the future when the matching predicate comes into play during the code section for example there are no invalid indices.

I believe this is Module::add_type which is where indices are validated and afterwards indices don't need validation.

I believe this is Module::add_type which is where indices are validated and afterwards indices don't need validation.

Yeah, that's exactly where matches() gets called:

<wasmparser::readers::core::types::SubType as wasmparser::readers::core::types::Matches>::matches types.rs:763 wasmparser::validator::core::Module::check_subtype core.rs:525 wasmparser::validator::core::Module::add_type core.rs:507

Ah ok I see that now, but I still think that this Result isn't necessary. For example if check_structural_type is moved above the subtype check, then everything is guaranteed to be valid while performing a subtype check?

Thanks! Updated.

crates/wasmparser/src/readers/core/types.rs

alexcrichton · 2023-07-03T15:13:11Z

crates/wasmparser/src/readers/core/types.rs

+                StructuralType::Func(_) => Ok(true),
+                _ => Ok(false),
+            },
+            (a, b) => Ok(a == b),


I've mentioned this before, but personally I'm not a fan of catch-all clauses like this. This is very easy to forget to update if a new variant is added because of #[derive(PartialEq)] which can lead to further bugs in behavior. Where possible I'd recommend exhaustively matching remaining cases, for example something like:

(a @ (HeapType::T1 | HeapType::T2), b) => Ok(a == b),

which explicitly documents the list of "catch-all" types being caught here and it's obvious there's no payload so equality can only be identity-based. That way if new types are added in the future this'll generate a compiler error with a non-exhaustive check.

because of #[derive(PartialEq)]

Not sure I understand what bad things can happen because of it. Everything else seems clear.

Also, updated the code according to your comment.

crates/wasmparser/src/readers/core/types.rs

alexcrichton · 2023-07-03T15:23:53Z

crates/wasm-mutate/src/info.rs

@@ -93,7 +93,7 @@ impl<'a> ModuleInfo<'a> {

                    // Save function types
                    for ty in reader {
-                        let typeinfo = TypeInfo::try_from(ty?).unwrap();
+                        let typeinfo = TypeInfo::try_from(ty?.structural_type).unwrap();


I think it would be best to update wasm-mutate a bit differently in this commit. Currently wasm-mutate doesn't have support for GC, and that's ok, but if given a binary with gc with these changes it runs a risk of "accepting" the binary and making it invalid. For example this is ignoring all fields other than structural_type.

I think it would be better to keep wasm-mutate as-is where it explicitly rejects gc-using binaries. For wasm-mutate that would look like handling the full SubType structure but returning errors if any fields are set that look like gc.

This reverts commit ab8af36.

# Conflicts: # crates/wasmparser/src/validator/types.rs

imikushin · 2023-07-06T21:59:27Z

@alexcrichton I believe I've addressed all your comments (so far).

Use `a == b` vs `*a == *b`: dereference is unnecessary since `PartialEq::eq(&self, other: &Rhs) -> bool` accepts references.

alexcrichton · 2023-07-07T14:28:25Z

Thanks!

This fixes an accidental regression from bytecodealliance#1059 where an invalid wasm file was causing a panic in wasmprinter, and wasmprinter shouldn't be panicking on invalid wasm files.

This fixes an accidental regression from #1059 where an invalid wasm file was causing a panic in wasmprinter, and wasmprinter shouldn't be panicking on invalid wasm files.

Releases recent changes such as: * More support for the gc proposal. [#1045](#1045) [#1059](#1059) * Support for resources throughout most tooling. [#1053](#1053) [#1068](#1068) [#1070](#1070) [#1082](#1082) [#1079](#1079) [#1083](#1083) [#1084](#1084) [#1105](#1105) [#1113](#1113) [#1116](#1116) * Support for `include` in WIT files. [#1054](#1054) [#1085](#1085) [#1088](#1088) * WIT worlds may now be rejected if the same interface can be "reached" as both an import and an export simultaneously. [#1081](#1081) [#1107](#1107) * Support for registry metadata in `wasm-tools metadata`. [#1060](#1060) * A new subcommand `wasm-tools component targets`. [#1089](#1089) * An `--out-dir` argument is supported on `wasm-tools component wit` to print the entire `Resolve` instead of just one package. [#1108](#1108) * Miscellaneous bug fixes and improvements. [#1061](#1061) [#1065](#1065) [#1074](#1074) [#1073](#1073) [#1078](#1078) [#1077](#1077) [#1072](#1072) [#1086](#1086) [#1091](#1091) [#1094](#1094) [#1114](#1114) [#1106](#1106)

imikushin mentioned this pull request Jun 12, 2023

GC: wasmparser: Fix gc/gc-ref.wat roundtrip test #1062

Merged

imikushin force-pushed the subtypes branch from bdd7ab6 to 3ca84b9 Compare June 12, 2023 22:28

fitzgen reviewed Jun 14, 2023

View reviewed changes

imikushin and others added 2 commits June 14, 2023 14:03

Accept an edit suggestion: punctuation.

3b28b89

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

Accept a suggestion: use .. in struct match expression

5099c9d

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

imikushin mentioned this pull request Jun 14, 2023

Add From implementations for structural types to StructuralType, SubType and Type #1071

Open

imikushin added 11 commits June 20, 2023 17:20

Address feedback: opcode

0350979

Address feedback: make supertype_idx an Option<u32>

ef15342

Address feedback: remove specific call order to encode subtype.

c78edbb

Broke the tests. Fixed them now.

4565224

Update matching logic.

2f38635

Add test cases.

0e8cb7b

cargo fmt

46d5514

Update matching logic.

a3dc082

Expand test coverage for subtypes

99d8457

Expand test coverage for subtypes

7b9c641

fitzgen approved these changes Jun 29, 2023

View reviewed changes

crates/wasm-encoder/src/core/types.rs Outdated Show resolved Hide resolved

crates/wasm-encoder/src/core/types.rs Outdated Show resolved Hide resolved

crates/wit-component/src/gc.rs Outdated Show resolved Hide resolved

tests/local/gc/gc-subtypes.wat Outdated Show resolved Hide resolved

imikushin and others added 6 commits June 30, 2023 10:46

Update crates/wasm-encoder/src/core/types.rs

3765018

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

Revert "Update crates/wasm-encoder/src/core/types.rs"

25384b9

This reverts commit 3765018.

Make sure test cases for invalid subtypes are addressed in `gc-subtyp…

acd7848

…es-invalid.wast`

Remove outdated comment.

9240c7f

Refactor TypeSection::subtype() removing unnecessary argument

ab09c16

Encode supertypes as a vector (as per spec).

3659a16

Fix "duplicate identifier for field" for subtype fields

ab8af36

Currently, field namespace is global (per module). But if a field of the same name is at the same position, that should be okay.

imikushin mentioned this pull request Jul 1, 2023

wasmparser: "duplicate identifier for field" in subtype #1092

Closed

alexcrichton reviewed Jul 3, 2023

View reviewed changes

imikushin added 8 commits July 3, 2023 15:56

Revert "Fix "duplicate identifier for field" for subtype fields"

692b14b

This reverts commit ab8af36.

Address more review feedback

e0e66e3

Address more review feedback: gc feature check

a7895f2

Address review feedback

b29d8e2

Address more review feedback

fb7ce21

Address more review feedback

3053c57

Merge branch 'main' into subtypes

85dd6d9

# Conflicts: # crates/wasmparser/src/validator/types.rs

Address review feedback

bdbd5fc

Minor tweaks

9911a68

Use `a == b` vs `*a == *b`: dereference is unnecessary since `PartialEq::eq(&self, other: &Rhs) -> bool` accepts references.

alexcrichton approved these changes Jul 7, 2023

View reviewed changes

alexcrichton merged commit e633e6f into bytecodealliance:main Jul 7, 2023

alexcrichton mentioned this pull request Jul 9, 2023

Don't panic in wasmprinter with non-function types #1114

Merged

alexcrichton mentioned this pull request Jul 11, 2023

Bump crate versions #1118

Merged

imikushin deleted the subtypes branch July 12, 2023 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GC: wasmparser: Subtypes #1059

GC: wasmparser: Subtypes #1059

imikushin commented Jun 9, 2023

imikushin commented Jun 14, 2023

fitzgen left a comment

fitzgen left a comment

imikushin commented Jun 30, 2023

alexcrichton Jul 3, 2023

imikushin Jul 4, 2023

alexcrichton Jul 5, 2023

imikushin Jul 5, 2023

alexcrichton Jul 6, 2023

imikushin Jul 6, 2023

alexcrichton Jul 3, 2023

imikushin Jul 6, 2023

imikushin Jul 6, 2023

alexcrichton Jul 3, 2023

imikushin Jul 6, 2023

imikushin commented Jul 6, 2023

alexcrichton commented Jul 7, 2023

GC: wasmparser: Subtypes #1059

GC: wasmparser: Subtypes #1059

Conversation

imikushin commented Jun 9, 2023

imikushin commented Jun 14, 2023

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen left a comment

Choose a reason for hiding this comment

imikushin commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imikushin commented Jul 6, 2023

alexcrichton commented Jul 7, 2023