Model constraints on StringNew children more precisely (#7193)

The stringref spec has a bug where the string.new... instructions that
take array reference parameters do not have the proper type annotations,
which means IRBuilder does not have a single type that it knows the
array reference must have. We previously fudged this by using `arrayref`
as the type constraint, but the fuzzer was able to generate a valid test
case that was parsed into invalid IR due to this imprecision.

Fix the bug by adding special constraint types for "any i8 array
reference" and "any i16 array reference" just for use with the offending
string.new instructions.

Fixes #7191.

Author: tlively

src/ir/child-typer.h

@@ -91,6 +91,18 @@ template<typename Subtype> struct ChildTyper : OverriddenVisitor<Subtype> {
     self().noteAnyTupleType(childp, arity);
   }
 
+  // Used only for string.new_lossy_utf8_array to work around a missing type
+  // annotation in the stringref spec.
+  void noteAnyI8ArrayReferenceType(Expression** childp) {
+    self().noteAnyI8ArrayReferenceType(childp);
+  }
+
+  // Used only for string.new_wtf16_array to work around a missing type
+  // annotation in the stringref spec.
+  void noteAnyI16ArrayReferenceType(Expression** childp) {
+    self().noteAnyI16ArrayReferenceType(childp);
+  }
+
   Type getLabelType(Name label) { return self().getLabelType(label); }
 
   void visitNop(Nop* curr) {}
@@ -992,15 +1004,15 @@ template<typename Subtype> struct ChildTyper : OverriddenVisitor<Subtype> {
     WASM_UNREACHABLE("unexpected op");
   }
 
-  void visitStringNew(StringNew* curr,
-                      std::optional<HeapType> ht = std::nullopt) {
+  void visitStringNew(StringNew* curr) {
     switch (curr->op) {
       case StringNewLossyUTF8Array:
+        noteAnyI8ArrayReferenceType(&curr->ref);
+        note(&curr->start, Type::i32);
+        note(&curr->end, Type::i32);
+        return;
       case StringNewWTF16Array:
-        if (!ht) {
-          ht = curr->ref->type.getHeapType();
-        }
-        note(&curr->ref, Type(*ht, Nullable));
+        noteAnyI16ArrayReferenceType(&curr->ref);
         note(&curr->start, Type::i32);
         note(&curr->end, Type::i32);
         return;

src/wasm/wasm-ir-builder.cpp

@@ -292,7 +292,16 @@ struct IRBuilder::ChildPopper
     size_t arity;
   };
 
-  struct Constraint : std::variant<Subtype, AnyType, AnyReference, AnyTuple> {
+  struct AnyI8ArrayReference {};
+
+  struct AnyI16ArrayReference {};
+
+  struct Constraint : std::variant<Subtype,
+                                   AnyType,
+                                   AnyReference,
+                                   AnyTuple,
+                                   AnyI8ArrayReference,
+                                   AnyI16ArrayReference> {
     std::optional<Type> getSubtype() const {
       if (auto* subtype = std::get_if<Subtype>(this)) {
         return subtype->bound;
@@ -301,6 +310,12 @@ struct IRBuilder::ChildPopper
     }
     bool isAnyType() const { return std::get_if<AnyType>(this); }
     bool isAnyReference() const { return std::get_if<AnyReference>(this); }
+    bool isAnyI8ArrayReference() const {
+      return std::get_if<AnyI8ArrayReference>(this);
+    }
+    bool isAnyI16ArrayReference() const {
+      return std::get_if<AnyI16ArrayReference>(this);
+    }
     std::optional<size_t> getAnyTuple() const {
       if (auto* tuple = std::get_if<AnyTuple>(this)) {
         return tuple->arity;
@@ -356,6 +371,14 @@ struct IRBuilder::ChildPopper
       children.push_back({childp, {AnyTuple{arity}}});
     }
 
+    void noteAnyI8ArrayReferenceType(Expression** childp) {
+      children.push_back({childp, {AnyI8ArrayReference{}}});
+    }
+
+    void noteAnyI16ArrayReferenceType(Expression** childp) {
+      children.push_back({childp, {AnyI16ArrayReference{}}});
+    }
+
     Type getLabelType(Name label) {
       WASM_UNREACHABLE("labels should be explicitly provided");
     };
@@ -455,6 +478,26 @@ struct IRBuilder::ChildPopper
             needUnreachableFallback = true;
             break;
           }
+        } else if (constraint.isAnyI8ArrayReference()) {
+          bool isI8Array =
+            type.isRef() && type.getHeapType().isArray() &&
+            type.getHeapType().getArray().element.packedType == Field::i8;
+          bool isNone =
+            type.isRef() && type.getHeapType().isMaybeShared(HeapType::none);
+          if (!isI8Array && !isNone && type != Type::unreachable) {
+            needUnreachableFallback = true;
+            break;
+          }
+        } else if (constraint.isAnyI16ArrayReference()) {
+          bool isI16Array =
+            type.isRef() && type.getHeapType().isArray() &&
+            type.getHeapType().getArray().element.packedType == Field::i16;
+          bool isNone =
+            type.isRef() && type.getHeapType().isMaybeShared(HeapType::none);
+          if (!isI16Array && !isNone && type != Type::unreachable) {
+            needUnreachableFallback = true;
+            break;
+          }
         } else {
           WASM_UNREACHABLE("unexpected constraint");
         }
@@ -601,13 +644,6 @@ struct IRBuilder::ChildPopper
     return popConstrainedChildren(children);
   }
 
-  Result<> visitStringNew(StringNew* curr,
-                          std::optional<HeapType> ht = std::nullopt) {
-    std::vector<Child> children;
-    ConstraintCollector{builder, children}.visitStringNew(curr, ht);
-    return popConstrainedChildren(children);
-  }
-
   Result<> visitStringEncode(StringEncode* curr,
                              std::optional<HeapType> ht = std::nullopt) {
     std::vector<Child> children;
@@ -1951,11 +1987,7 @@ Result<> IRBuilder::makeStringNew(StringNewOp op) {
     push(builder.makeStringNew(op, curr.ref));
     return Ok{};
   }
-  // There's no type annotation on these instructions due to a bug in the
-  // stringref proposal, so we just fudge it and pass `array` instead of a
-  // defined heap type. This will allow us to pop a child with an invalid
-  // array type, but that's just too bad.
-  CHECK_ERR(ChildPopper{*this}.visitStringNew(&curr, HeapType::array));
+  CHECK_ERR(visitStringNew(&curr));
   push(builder.makeStringNew(op, curr.ref, curr.start, curr.end));
   return Ok{};
 }