[Wasm GC] Add test/spec/br_on_null.wast and validation fixes for it (#3623)

This adds ValidationBuilder which can allow sharing of builder code that also
validates, between the text and binary parsers. In general we share that code in
the validator, but the validator can only run once IR exists, and in some cases we
can't even emit valid IR structure at all.

5 files changed, 76 insertions, 28 deletions

diff --git a/src/passes/Inlining.cpp b/src/passes/Inlining.cpp
index d0a69ea96..727e94d72 100644
--- a/src/passes/Inlining.cpp
+++ b/src/passes/Inlining.cpp
@@ -63,6 +63,7 @@ struct FunctionInfo {
   // See pass.h for how defaults for these options were chosen.
   bool worthInlining(PassOptions& options) {
     // Until we have proper support for try-delegate, ignore such functions.
+    // FIXME https://github.com/WebAssembly/binaryen/issues/3634
     if (hasTryDelegate) {
       return false;
     }
diff --git a/src/wasm-builder.h b/src/wasm-builder.h
index 2277fed93..3ca3d9daf 100644
--- a/src/wasm-builder.h
+++ b/src/wasm-builder.h
@@ -18,6 +18,7 @@
 #define wasm_wasm_builder_h
 
 #include "ir/manipulation.h"
+#include "parsing.h"
 #include "wasm.h"
 
 namespace wasm {
@@ -39,7 +40,7 @@ class Builder {
 public:
   Builder(Module& wasm) : wasm(wasm) {}
 
-  // make* functions, other globals
+  // make* functions create an expression instance.
 
   static std::unique_ptr<Function> makeFunction(Name name,
                                                 Signature sig,
@@ -1116,6 +1117,65 @@ public:
   }
 };
 
+// This class adds methods that first inspect the input. They may not have fully
+// comprehensive error checking, when that can be left to the validator; the
+// benefit of the validate* methods is that they can share code between the
+// text and binary format parsers, for handling certain situations in the
+// input which preclude even creating valid IR, which the validator depends
+// on.
+class ValidatingBuilder : public Builder {
+  size_t line = -1, col = -1;
+
+public:
+  ValidatingBuilder(Module& wasm, size_t line) : Builder(wasm), line(line) {}
+  ValidatingBuilder(Module& wasm, size_t line, size_t col)
+    : Builder(wasm), line(line), col(col) {}
+
+  Expression* validateAndMakeBrOn(BrOnOp op,
+                                  Name name,
+                                  Expression* ref,
+                                  Expression* rtt = nullptr) {
+    if (op == BrOnCast) {
+      if (rtt->type == Type::unreachable) {
+        // An unreachable rtt is not supported: the text and binary formats do
+        // not provide the type, so if it's unreachable we should not even
+        // create a br_on_cast in such a case, as we'd have no idea what it
+        // casts to.
+        return makeSequence(makeDrop(ref), rtt);
+      }
+    }
+    if (op == BrOnNull) {
+      if (!ref->type.isRef() && ref->type != Type::unreachable) {
+        throw ParseException("Invalid ref for br_on_null", line, col);
+      }
+    }
+    return makeBrOn(op, name, ref, rtt);
+  }
+
+  template<typename T>
+  Expression* validateAndMakeCallRef(Expression* target,
+                                     const T& args,
+                                     bool isReturn = false) {
+    if (!target->type.isRef()) {
+      if (target->type == Type::unreachable) {
+        // An unreachable target is not supported. Similiar to br_on_cast, just
+        // emit an unreachable sequence, since we don't have enough information
+        // to create a full call_ref.
+        auto* block = makeBlock(args);
+        block->list.push_back(target);
+        block->finalize(Type::unreachable);
+        return block;
+      }
+      throw ParseException("Non-reference type for a call_ref", line, col);
+    }
+    auto heapType = target->type.getHeapType();
+    if (!heapType.isSignature()) {
+      throw ParseException("Invalid reference type for a call_ref", line, col);
+    }
+    return makeCallRef(target, args, heapType.getSignature().results, isReturn);
+  }
+};
+
 } // namespace wasm
 
 #endif // wasm_wasm_builder_h
diff --git a/src/wasm-s-parser.h b/src/wasm-s-parser.h
index 25efd1fc1..9fb9d4389 100644
--- a/src/wasm-s-parser.h
+++ b/src/wasm-s-parser.h
@@ -238,13 +238,16 @@ private:
   Expression* makeLoop(Element& s);
   Expression* makeCall(Element& s, bool isReturn);
   Expression* makeCallIndirect(Element& s, bool isReturn);
-  template<class T>
-  void parseCallOperands(Element& s, Index i, Index j, T* call) {
+  template<class T> void parseOperands(Element& s, Index i, Index j, T& list) {
     while (i < j) {
-      call->operands.push_back(parseExpression(s[i]));
+      list.push_back(parseExpression(s[i]));
       i++;
     }
   }
+  template<class T>
+  void parseCallOperands(Element& s, Index i, Index j, T* call) {
+    parseOperands(s, i, j, call->operands);
+  }
   enum class LabelType { Break, Exception };
   Name getLabel(Element& s, LabelType labelType = LabelType::Break);
   Expression* makeBreak(Element& s);
diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
index ebc5888a0..c0d7ab36e 100644
--- a/src/wasm/wasm-binary.cpp
+++ b/src/wasm/wasm-binary.cpp
@@ -6193,7 +6193,7 @@ bool WasmBinaryBuilder::maybeVisitBrOn(Expression*& out, uint32_t code) {
     rtt = popNonVoidExpression();
   }
   auto* ref = popNonVoidExpression();
-  out = Builder(wasm).makeBrOn(op, name, ref, rtt);
+  out = ValidatingBuilder(wasm, pos).validateAndMakeBrOn(op, name, ref, rtt);
   return true;
 }
 
diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index 87a6e7abd..15b9cd5cb 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -2533,21 +2533,11 @@ Expression* SExpressionWasmBuilder::makeTupleExtract(Element& s) {
 }
 
 Expression* SExpressionWasmBuilder::makeCallRef(Element& s, bool isReturn) {
-  auto ret = allocator.alloc<CallRef>();
-  parseCallOperands(s, 1, s.size() - 1, ret);
-  ret->target = parseExpression(s[s.size() - 1]);
-  ret->isReturn = isReturn;
-  if (!ret->target->type.isRef()) {
-    throw ParseException("Non-reference type for a call_ref", s.line, s.col);
-  }
-  auto heapType = ret->target->type.getHeapType();
-  if (!heapType.isSignature()) {
-    throw ParseException(
-      "Invalid reference type for a call_ref", s.line, s.col);
-  }
-  auto sig = heapType.getSignature();
-  ret->finalize(sig.results);
-  return ret;
+  std::vector<Expression*> operands;
+  parseOperands(s, 1, s.size() - 1, operands);
+  auto* target = parseExpression(s[s.size() - 1]);
+  return ValidatingBuilder(wasm, s.line, s.col)
+    .validateAndMakeCallRef(target, operands, isReturn);
 }
 
 Expression* SExpressionWasmBuilder::makeI31New(Element& s) {
@@ -2585,17 +2575,11 @@ Expression* SExpressionWasmBuilder::makeBrOn(Element& s, BrOnOp op) {
   auto name = getLabel(*s[1]);
   auto* ref = parseExpression(*s[2]);
   Expression* rtt = nullptr;
-  Builder builder(wasm);
   if (op == BrOnCast) {
     rtt = parseExpression(*s[3]);
-    if (rtt->type == Type::unreachable) {
-      // An unreachable rtt is not supported: the text format does not provide
-      // the type, so if it's unreachable we should not even create a br_on_cast
-      // in such a case, as we'd have no idea what it casts to.
-      return builder.makeSequence(builder.makeDrop(ref), rtt);
-    }
   }
-  return builder.makeBrOn(op, name, ref, rtt);
+  return ValidatingBuilder(wasm, s.line, s.col)
+    .validateAndMakeBrOn(op, name, ref, rtt);
 }
 
 Expression* SExpressionWasmBuilder::makeRttCanon(Element& s) {