Relax the block check in OperatorsReader<'_>

This commit relaxes a check added in bytecodealliance#2134 which maintains a stack of
frame kinds in the operators reader, in addition to the validator. The
goal of bytecodealliance#2134 was to ensure that spec-wise-syntactically-invalid-modules
are caught in the parser without the need of the validator, but
investigation in bytecodealliance#2180 has shown that this is a source of at least some
of a performance regression. The change here is to relax the check to
still be able to pass spec tests while making such infrastructure
cheaper.

The reader now maintains just a small `depth: u32` counter instead of a
stack of kinds. This means that the reader can still catch invalid
modules such as instructions-after-`end`, but the validator is required
to handle situations such as `else` outside of an `if` block.

This required some adjustments to tests as well as some workarounds for
the upstream spec tests that assert legacy exception-handling
instructions are malformed, not invalid.

Author: alexcrichton

ci/generate-spec-tests.rs

@@ -44,11 +44,21 @@ fn copy_test(src: &Path, dst: &Path) {
     contents.push_str(";;      --assert default \\\n");
 
     // Allow certain assert_malformed tests to be interpreted as assert_invalid
-    if src.ends_with("binary.wast") || src.ends_with("global.wast") || src.ends_with("select.wast")
+    if src.ends_with("binary.wast")
+        || src.ends_with("global.wast")
+        || src.ends_with("select.wast")
+        || src.ends_with("try_table.wast")
     {
         contents.push_str(";;      --assert permissive \\\n");
     }
 
+    // For this test it has legacy instructions which, for roundabout reasons,
+    // are attempted to be printed in the folded format but that doesn't work.
+    // Exclude this test for now.
+    if src.ends_with("try_table.wast") {
+        contents.push_str(";;      --assert no-test-folded \\\n");
+    }
+
     contents.push_str(";;      --snapshot tests/snapshots \\\n");
 
     // This test specifically tests various forms of unicode which are

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

@@ -336,7 +336,13 @@ crate::for_each_operator!(define_operator);
 #[derive(Clone)]
 pub struct OperatorsReader<'a> {
     reader: BinaryReader<'a>,
-    blocks: Vec<FrameKind>,
+    /// Block depth this reader is currently operating at.
+    ///
+    /// Increments for instructions like `block` and decrements for instructions
+    /// like `end`. Used to rule out syntactically invalid modules (as defined
+    /// by the wasm spec currently) in a cheap but not 100% rigorous fashion.
+    /// More checks are still performed in the validator about block depth.
+    depth: u32,
     data_index_occurred: Option<usize>,
 }
 
@@ -345,7 +351,7 @@ impl<'a> OperatorsReader<'a> {
     pub fn new(reader: BinaryReader<'a>) -> OperatorsReader<'a> {
         OperatorsReader {
             reader,
-            blocks: vec![FrameKind::Block],
+            depth: 1,
             data_index_occurred: None,
         }
     }
@@ -377,7 +383,7 @@ impl<'a> OperatorsReader<'a> {
     }
 
     fn ensure_stack_empty(&self) -> Result<()> {
-        if !self.blocks.is_empty() {
+        if self.depth != 0 {
             bail!(
                 self.original_position(),
                 "control frames remain at end of function body or expression"
@@ -410,33 +416,6 @@ impl<'a> OperatorsReader<'a> {
         Ok((self.read()?, pos))
     }
 
-    fn enter(&mut self, k: FrameKind) {
-        self.blocks.push(k)
-    }
-
-    fn expect_block(&mut self, k: FrameKind, found: &str) -> Result<()> {
-        match self.blocks.last() {
-            None => bail!(
-                self.original_position(),
-                "empty stack found where {:?} expected",
-                k
-            ),
-            Some(x) if *x == k => Ok(()),
-            Some(_) => bail!(
-                self.original_position(),
-                "`{}` found outside `{:?}` block",
-                found,
-                k
-            ),
-        }
-    }
-
-    fn end(&mut self) -> Result<()> {
-        assert!(!self.blocks.is_empty());
-        self.blocks.pop();
-        Ok(())
-    }
-
     /// Visit the next available operator with the specified [`VisitOperator`] instance.
     ///
     /// Note that this does not implicitly propagate any additional information such as instruction
@@ -486,7 +465,7 @@ impl<'a> OperatorsReader<'a> {
     where
         T: VisitOperator<'a>,
     {
-        if self.blocks.is_empty() {
+        if self.depth == 0 {
             bail!(
                 self.original_position(),
                 "operators remaining after end of function body or expression"
@@ -498,46 +477,28 @@ impl<'a> OperatorsReader<'a> {
             0x00 => visitor.visit_unreachable(),
             0x01 => visitor.visit_nop(),
             0x02 => {
-                self.enter(FrameKind::Block);
+                self.depth += 1;
                 visitor.visit_block(self.reader.read_block_type()?)
             }
             0x03 => {
-                self.enter(FrameKind::Loop);
+                self.depth += 1;
                 visitor.visit_loop(self.reader.read_block_type()?)
             }
             0x04 => {
-                self.enter(FrameKind::If);
+                self.depth += 1;
                 visitor.visit_if(self.reader.read_block_type()?)
             }
-            0x05 => {
-                self.expect_block(FrameKind::If, "else")?;
-                visitor.visit_else()
-            }
+            0x05 => visitor.visit_else(),
             0x06 => {
-                if !self.reader.legacy_exceptions() {
-                    bail!(
-                        pos,
-                        "legacy_exceptions feature required for try instruction"
-                    );
-                }
-                self.enter(FrameKind::LegacyTry);
+                self.depth += 1;
                 visitor.visit_try(self.reader.read_block_type()?)
             }
-            0x07 => {
-                if !self.reader.legacy_exceptions() {
-                    bail!(
-                        pos,
-                        "legacy_exceptions feature required for catch instruction"
-                    );
-                }
-                self.expect_block(FrameKind::LegacyTry, "catch")?;
-                visitor.visit_catch(self.reader.read_var_u32()?)
-            }
+            0x07 => visitor.visit_catch(self.reader.read_var_u32()?),
             0x08 => visitor.visit_throw(self.reader.read_var_u32()?),
             0x09 => visitor.visit_rethrow(self.reader.read_var_u32()?),
             0x0a => visitor.visit_throw_ref(),
             0x0b => {
-                self.end()?;
+                self.depth -= 1;
                 visitor.visit_end()
             }
             0x0c => visitor.visit_br(self.reader.read_var_u32()?),
@@ -558,20 +519,10 @@ impl<'a> OperatorsReader<'a> {
             0x14 => visitor.visit_call_ref(self.reader.read()?),
             0x15 => visitor.visit_return_call_ref(self.reader.read()?),
             0x18 => {
-                self.expect_block(FrameKind::LegacyTry, "delegate")?;
-                self.blocks.pop();
+                self.depth -= 1;
                 visitor.visit_delegate(self.reader.read_var_u32()?)
             }
-            0x19 => {
-                if !self.reader.legacy_exceptions() {
-                    bail!(
-                        pos,
-                        "legacy_exceptions feature required for catch_all instruction"
-                    );
-                }
-                self.expect_block(FrameKind::LegacyTry, "catch_all")?;
-                visitor.visit_catch_all()
-            }
+            0x19 => visitor.visit_catch_all(),
             0x1a => visitor.visit_drop(),
             0x1b => visitor.visit_select(),
             0x1c => {
@@ -590,7 +541,7 @@ impl<'a> OperatorsReader<'a> {
                 }
             }
             0x1f => {
-                self.enter(FrameKind::TryTable);
+                self.depth += 1;
                 visitor.visit_try_table(self.reader.read()?)
             }
 

src/bin/wasm-tools/wast.rs

@@ -251,6 +251,12 @@ impl Opts {
                     // two error messages are swapped to `assert_invalid`.
                     "malformed mutability",
                     "integer too large",
+                    // The upstream specification requires that the legacy
+                    // exceptions proposal is textually invalid (e.g.
+                    // `assert_malformed`). This crate supports it though as
+                    // "just another proposal", so it's not malformed but it's
+                    // invalid.
+                    "unexpected token",
                 ];
                 if self.assert(Assert::Permissive) && permissive_error_messages.contains(&message) {
                     return self.test_wast_directive(

tests/cli/invalid.wast

@@ -1,12 +1,12 @@
-;; RUN: wast --assert default --snapshot tests/snapshots %
+;; RUN: wast --assert default,no-test-folded --snapshot tests/snapshots %
 
 (assert_invalid
   (module
     (table 1 funcref)
     (func table.init 0 100))
   "unknown elem segment")
 
-(assert_malformed
+(assert_invalid
   (module
     (func else))
-  "`else` found outside `If` block")
+  "else found outside of an `if` block ")

tests/cli/legacy-exceptions/disabled.wast

@@ -0,0 +1,13 @@
+;; RUN: wast %
+
+(assert_invalid
+  (module (func try end))
+  "legacy exceptions support is not enabled")
+
+(assert_invalid
+  (module (func catch 0))
+  "legacy exceptions support is not enabled")
+
+(assert_invalid
+  (module (func catch_all))
+  "legacy exceptions support is not enabled")

tests/cli/print-no-panic-dangling-else.wat

@@ -1,4 +1,4 @@
-;; FAIL: print %
+;; RUN: print %
 
 (module
     (func else)

tests/cli/print-no-panic-dangling-else.wat.stderr

@@ -1,3 +1,6 @@
 (module
   (type (;0;) (func))
   (func (;0;) (type 0)
+  else
+  )
+)

tests/cli/print-no-panic-dangling-else.wat.stdout

@@ -1,5 +1,7 @@
 ;; RUN: wast \
 ;;      --assert default \
+;;      --assert permissive \
+;;      --assert no-test-folded \
 ;;      --snapshot tests/snapshots \
 ;;      --ignore-error-messages \
 ;;      --features=wasm2,exceptions,tail-call \

tests/cli/spec/proposals/exception-handling/try_table.wast

@@ -1,5 +1,7 @@
 ;; RUN: wast \
 ;;      --assert default \
+;;      --assert permissive \
+;;      --assert no-test-folded \
 ;;      --snapshot tests/snapshots \
 ;;      --ignore-error-messages \
 ;;      --features=wasm3 \

tests/cli/spec/proposals/wasm-3.0/try_table.wast

@@ -9,11 +9,11 @@
       "text": "unknown elem segment"
     },
     {
-      "type": "assert_malformed",
+      "type": "assert_invalid",
       "line": 10,
       "filename": "invalid.1.wasm",
       "module_type": "binary",
-      "text": "`else` found outside `If` block"
+      "text": "else found outside of an `if` block "
     }
   ]
 }