wasmparser: Fix behavior of branching to labels and subtyping

Addresses issues discussed in WebAssembly/gc#516

crates/wasmparser/src/validator/operators.rs

@@ -187,13 +187,6 @@ impl MaybeType {
             Self::Bot | Self::HeapBot => None,
         }
     }
-
-    fn or(self, alternative: impl Into<MaybeType>) -> MaybeType {
-        match self {
-            Self::Type(_) => self,
-            Self::Bot | Self::HeapBot => alternative.into(),
-        }
-    }
 }
 
 impl OperatorValidator {
@@ -409,11 +402,6 @@ impl<R> DerefMut for OperatorValidatorTemp<'_, '_, R> {
     }
 }
 
-enum PopPushBottomBehavior {
-    PushBottom,
-    OrExpected,
-}
-
 impl<'resources, R> OperatorValidatorTemp<'_, 'resources, R>
 where
     R: WasmModuleResources,
@@ -482,41 +470,12 @@ where
     fn pop_push_label_types(
         &mut self,
         label_types: impl PreciseIterator<Item = ValType>,
-        bottom_behavior: PopPushBottomBehavior,
     ) -> Result<()> {
-        // When popping off the expected types from the stack for a branch
-        // label, the actual types might be subtypes of those expected types,
-        // and we don't want to accidentally erase that type information by
-        // pushing the label's expected types, so we have to save the actual
-        // popped types.
-
-        debug_assert!(self.popped_types_tmp.is_empty());
-        self.popped_types_tmp.reserve(label_types.len());
-        for expected_ty in label_types.rev() {
-            let actual_ty = self.pop_operand(Some(expected_ty))?;
-
-            let actual_ty = match bottom_behavior {
-                // However, in addition, if we pop bottom, then we generally
-                // don't want to push bottom again! That would allow for the
-                // "same" stack slot to be used as an `i32` in one unreachable
-                // instruction and as an `i64` in a different unreachable
-                // instruction, which leads to invalid test cases to be treated
-                // as valid.
-                PopPushBottomBehavior::OrExpected => actual_ty.or(expected_ty),
-                // The only exception to the above is when we are handling
-                // `br_table` and we aren't logically pushing the *same* value
-                // back to the stack again, but instead resetting the stack to
-                // its previous state, as if we didn't branch to the previous
-                // target. In this case, we can interpret bottom as `i32` for
-                // one branch target, and as `i64` for another branch target, so
-                // we really do want to re-push bottom again.
-                PopPushBottomBehavior::PushBottom => actual_ty,
-            };
-
-            self.popped_types_tmp.push(actual_ty);
+        for ty in label_types.clone().rev() {
+            self.pop_operand(Some(ty))?;
         }
-        for ty in self.inner.popped_types_tmp.drain(..).rev() {
-            self.inner.operands.push(ty.into());
+        for ty in label_types {
+            self.push_operand(ty)?;
         }
         Ok(())
     }
@@ -1473,7 +1432,7 @@ where
         self.pop_operand(Some(ValType::I32))?;
         let (ty, kind) = self.jump(relative_depth)?;
         let label_types = self.label_types(ty, kind)?;
-        self.pop_push_label_types(label_types, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_types)?;
         Ok(())
     }
     fn visit_br_table(&mut self, table: BrTable) -> Self::Output {
@@ -1490,7 +1449,16 @@ where
                     "type mismatch: br_table target labels have different number of types"
                 );
             }
-            self.pop_push_label_types(label_tys, PopPushBottomBehavior::PushBottom)?;
+
+            debug_assert!(self.popped_types_tmp.is_empty());
+            self.popped_types_tmp.reserve(label_tys.len());
+            for expected_ty in label_tys.rev() {
+                let actual_ty = self.pop_operand(Some(expected_ty))?;
+                self.popped_types_tmp.push(actual_ty);
+            }
+            for ty in self.inner.popped_types_tmp.drain(..).rev() {
+                self.inner.operands.push(ty.into());
+            }
         }
         for ty in default_types.rev() {
             self.pop_operand(Some(ty))?;
@@ -2471,7 +2439,7 @@ where
         };
         let (ft, kind) = self.jump(relative_depth)?;
         let label_types = self.label_types(ft, kind)?;
-        self.pop_push_label_types(label_types, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_types)?;
         self.push_operand(ref_ty)?;
         Ok(())
     }
@@ -2506,7 +2474,7 @@ where
             ),
         }
 
-        self.pop_push_label_types(label_types, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_types)?;
         Ok(())
     }
     fn visit_ref_is_null(&mut self) -> Self::Output {
@@ -3940,7 +3908,7 @@ where
             ),
         };
 
-        self.pop_push_label_types(label_types, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_types)?;
         let diff_ty = RefType::difference(from_ref_type, to_ref_type);
         self.push_operand(diff_ty)?;
         Ok(())
@@ -3984,7 +3952,7 @@ where
             ),
         }
 
-        self.pop_push_label_types(label_tys, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_tys)?;
         self.push_operand(to_ref_type)?;
         Ok(())
     }

tests/local/gc/invalid.wast

@@ -0,0 +1,31 @@
+;; https://github.com/WebAssembly/gc/issues/516
+
+(assert_invalid
+  (module
+    (func (param anyref) (result anyref)
+      ref.null struct
+      local.get 0
+      ;; The label pushes an anyref, even though we really have a structref.
+      br_on_null 0
+      drop
+      br_on_cast_fail 0  structref structref
+    )
+  )
+  "type mismatch: expected structref, found anyref"
+)
+
+(assert_invalid
+  (module
+    (func (param anyref) (result anyref)
+      ref.null struct
+      ;; Adding an `unreachable` shouldn't change the fact that the `br_on_null`
+      ;; pushes an `anyref` and the `br_on_cast_fail` expects a `structref`.
+      unreachable
+      local.get 0
+      br_on_null 0
+      drop
+      br_on_cast_fail 0  structref structref
+    )
+  )
+ "type mismatch: expected structref, found anyref"
+ )

tests/local/unreachable-valid.wast

@@ -0,0 +1,11 @@
+(module
+  (func (export "f")
+    f32.const 1.0
+    f32.const 2.0
+    br 0
+    i32.add
+    drop
+  )
+)
+
+(assert_trap (invoke "f") "unreachable")

tests/snapshots/local/unreachable-valid.wast/0.print

@@ -0,0 +1,11 @@
+(module
+  (type (;0;) (func))
+  (func (;0;) (type 0)
+    f32.const 0x1p+0 (;=1;)
+    f32.const 0x1p+1 (;=2;)
+    br 0 (;@0;)
+    i32.add
+    drop
+  )
+  (export "f" (func 0))
+)