Roundtrip non-malformed test cases through wasmparser / wasmprinter #2148

keithw · 2025-04-15T22:14:12Z

This PR modifies wasmprinter and wasm-encoder to support roundtripping all non-malformed examples in the testsuite. The main changes were to

handle multi-value typed select (currently invalid, but not malformed), and
~~ensure that component printer doesn't use forward-referring name references in the text format (these are rejected by the component parser)~~

It then modifies the wast subcommand to run each assert_invalid test roundtrip through the text format and back.

keithw · 2025-04-15T22:15:13Z

This is the last PR in my series. I think it is something of an extension of #2146 so no objections to landing that first.

alexcrichton · 2025-04-16T14:31:56Z

crates/wasmparser/src/lib.rs

@@ -330,7 +330,7 @@ macro_rules! _for_each_operator_group {
            // reference-types
            // https://github.com/WebAssembly/reference-types
            @reference_types {
-                TypedSelect { ty: $crate::ValType } => visit_typed_select (arity 3 -> 1)
+                TypedSelect { tys: Vec<$crate::ValType> } => visit_typed_select (arity select -> select)


To me this is another example which is sort of a step-too-far in following the spec exactly as-is. Primarily from a performance perspective this means that any time this instruction is parsed it ends up doing a heap allocation where previously it didn't, and the only purpose is to check the length of the vector and ensure that it's one.

This is I think another example of where the spec is written in a way that doesn't really consider the consequence whether something is malformed or invalid. To me this would ideally result in a change to the binary grammar of the spec to say that a non-1-length typed select is a binary syntax error rather than a validation error.

For now could the select.wast test be excluded from the round-trip of assert_invalid directives?

Fair enough... what do you think of the just-pushed approach (where visit_typed_select_multi is another visitor function off to the side)?

I really would love to keep the invariant that every parseable module can be roundtripped (especially since we're so close, and my group is trying to write a Wasm text editor that enforces the syntax), but I agree it shouldn't affect performance.

Clever solution! I'm down for keeping that. Mind adding a comment to TypedSelectMulti as to its purpose though? (e.g. it's always an invalid instruction but it's there to round-trip the binary format)

Sure! Done.

crates/wasmprinter/src/lib.rs

alexcrichton · 2025-04-16T14:52:05Z

crates/wasmprinter/src/lib.rs

@@ -156,7 +157,7 @@ struct State {
    core: CoreState,
    #[cfg(feature = "component-model")]
    component: ComponentState,
-    custom_section_place: Option<&'static str>,
+    custom_section_place: Option<(&'static str, usize)>,


This new usize variable is to me pretty subtle and is something I'd prefer to avoid. Locally I see that this test is what motivated this change. Would it make sense to exclude module binary from both binary roundtripping and text roundtripping? (for similar reasons effectively). That would result in there being no more need for this change?

Yeah, the issue is that if a section is empty, you don't want to update the "after xxx" custom section place because nothing got printed (and so the module won't roundtrip if you do this twice).

Given that this gets us to 100% binary-to-binary roundtrippability of all modules that originated in text, and 100% text-to-text roundtrippability of all modules that originated in binary, I would love to find a way to keep that invariant -- it feels like a really nice property to have and ensure. E.g. it did successfully catch a bug in that the wasmprinter was printing after element instead of after elem.

Are you open to some alternative implementation strategy here? We basically just need to make sure that the custom section place doesn't get updated if the section was empty -- I don't have any strong feelings about how we do it.

Ok that all makes sense, and I agree it's nice to catch the after element bug as well. Given all that let's leave this as-is, but can you add a comment here indicating what the two fields of the Option are and how the line number is being used to detect empty sections?

crates/wasmprinter/src/lib.rs

Handle multi-value typed select, even though this is currently invalid (but not malformed). Not handled: invalid components that use forward-referring name references (which the component parser will reject)

alexcrichton · 2025-04-17T18:43:13Z

crates/wasmparser/src/validator/operators.rs

+        if tys.len() != 1 {
+            bail!(self.offset, "invalid result arity");
+        }
+        self.visit_typed_select(tys.pop().unwrap())


Could this debug_assert! that tys.len() != 1 and then always return with bail!?

Sure! Done.

alexcrichton · 2025-04-17T18:43:34Z

src/bin/wasm-tools/dump.rs

+                    match stringify!($visit).strip_prefix("visit_").unwrap() {
+			"typed_select_multi" => "typed_select",
+			other => other,
+		    },


Since this is just a debugging utility I think it's reasonable to print typed_select_multi in this case

alexcrichton · 2025-04-17T18:49:35Z

crates/wasmprinter/src/lib.rs

@@ -156,7 +157,7 @@ struct State {
    core: CoreState,
    #[cfg(feature = "component-model")]
    component: ComponentState,
-    custom_section_place: Option<&'static str>,
+    custom_section_place: Option<(&'static str, usize)>,


Ok that all makes sense, and I agree it's nice to catch the after element bug as well. Given all that let's leave this as-is, but can you add a comment here indicating what the two fields of the Option are and how the line number is being used to detect empty sections?

crates/wasmprinter/src/lib.rs

alexcrichton · 2025-04-17T18:50:47Z

crates/wasmparser/src/lib.rs

@@ -330,7 +330,7 @@ macro_rules! _for_each_operator_group {
            // reference-types
            // https://github.com/WebAssembly/reference-types
            @reference_types {
-                TypedSelect { ty: $crate::ValType } => visit_typed_select (arity 3 -> 1)
+                TypedSelect { tys: Vec<$crate::ValType> } => visit_typed_select (arity select -> select)


Clever solution! I'm down for keeping that. Mind adding a comment to TypedSelectMulti as to its purpose though? (e.g. it's always an invalid instruction but it's there to round-trip the binary format)

keithw · 2025-04-17T20:31:38Z

🙏 🎆 Thank you!

alexcrichton · 2025-04-17T20:32:47Z

And of course thank you for working on this (and working with me on the review)!

alexcrichton reviewed Apr 16, 2025

View reviewed changes

alexcrichton mentioned this pull request Apr 16, 2025

Release wasm-tools 1.229.0 #2149

Merged

keithw force-pushed the roundtrip-invalid-tests branch 3 times, most recently from 58969d7 to 0d74fd0 Compare April 17, 2025 16:52

keithw added 2 commits April 17, 2025 10:38

wasmprinter/-encoder: support roundtripping non-malformed modules

d7aaceb

Handle multi-value typed select, even though this is currently invalid (but not malformed). Not handled: invalid components that use forward-referring name references (which the component parser will reject)

wast.rs: roundtrip all non-malformed test cases

5c973b2

keithw force-pushed the roundtrip-invalid-tests branch from 0d74fd0 to 5c973b2 Compare April 17, 2025 17:38

wasmprinter lib.rs: clarify comment

456cc16

alexcrichton approved these changes Apr 17, 2025

View reviewed changes

keithw added 2 commits April 17, 2025 12:55

Respond to review comments

4b414cd

wast.rs: restore dropped line of comment

e896a63

alexcrichton added this pull request to the merge queue Apr 17, 2025

Merged via the queue into bytecodealliance:main with commit 00c7efd Apr 17, 2025
32 checks passed

keithw deleted the roundtrip-invalid-tests branch April 17, 2025 20:31

Roundtrip non-malformed test cases through wasmparser / wasmprinter #2148

Roundtrip non-malformed test cases through wasmparser / wasmprinter #2148

Uh oh!

Conversation

keithw commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keithw commented Apr 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithw Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithw Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keithw commented Apr 17, 2025

Uh oh!

alexcrichton commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

keithw commented Apr 15, 2025 •

edited

Loading

keithw Apr 17, 2025 •

edited

Loading

keithw Apr 17, 2025 •

edited

Loading

alexcrichton commented Apr 17, 2025 •

edited

Loading