From 02372bf1345a5e17576c4482c6a85cc792e2e9b7 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Tue, 28 Jul 2026 17:58:02 -0700 Subject: [PATCH] Fix unreachable br_on_cast parse bug The br_on_cast* family of instructions take input and output type immediates. We do not store the input type in the IR, so IRBuilder has to check the type of the `ref` operand against the input type before the input type is lost. For br_on_cast_desc_eq{_fail} instructions with unreachable descriptor operands, we had a bug that would allow us to pop a `ref` operand with an incorrect type, which would then immediately fail that type check. This was incorrect because the unreachable `desc` operand should have created a polymorphic stack that allowed popping any valid type. Fix the bug by threading the expected input type through IRBuilder to ChildTyper, where we can note the proper type constraint on the `ref` operand. --- src/ir/child-typer.h | 17 +++--- src/parser/contexts.h | 4 +- src/wasm-ir-builder.h | 4 +- src/wasm/wasm-ir-builder.cpp | 53 ++++++++++++------- .../basic/unreachable-br-on-cast-desc.wast | 41 ++++++++++++++ 5 files changed, 88 insertions(+), 31 deletions(-) create mode 100644 test/lit/basic/unreachable-br-on-cast-desc.wast diff --git a/src/ir/child-typer.h b/src/ir/child-typer.h index 148cb7219ba..2ecabfbf65e 100644 --- a/src/ir/child-typer.h +++ b/src/ir/child-typer.h @@ -914,13 +914,15 @@ template struct ChildTyper : OverriddenVisitor { note(&curr->ref, VarRef{Nullable, VarDefHeapType{VarExactness{0u}, *ht}}); } - void visitBrOn(BrOn* curr, std::optional target = std::nullopt) { + void visitBrOn(BrOn* curr, + std::optional in = std::nullopt, + std::optional out = std::nullopt) { switch (curr->op) { case BrOnNull: case BrOnNonNull: // br_on(_non)_null is polymorphic over reference types and does not // take a type immediate. - assert(!target); + assert(!in && !out); // Polymorphic over heap types. note(&curr->ref, VarRef{Nullable, VarHeapType{0u}}); return; @@ -928,14 +930,15 @@ template struct ChildTyper : OverriddenVisitor { case BrOnCastFail: case BrOnCastDescEq: case BrOnCastDescEqFail: { - if (!target) { + assert(!!in == !!out); + if (!out) { assert(curr->castType.isRef()); - target = curr->castType; + out = curr->castType; + in = Type(out->getHeapType().getTop(), Nullable); } - auto top = target->getHeapType().getTop(); - note(&curr->ref, Type(top, Nullable)); + note(&curr->ref, *in); if (curr->op == BrOnCastDescEq || curr->op == BrOnCastDescEqFail) { - auto desc = target->getHeapType().getDescriptorType(); + auto desc = out->getHeapType().getDescriptorType(); assert(desc); note(&curr->desc, Type(*desc, Nullable)); } diff --git a/src/parser/contexts.h b/src/parser/contexts.h index c873bef7653..9602a17ce1f 100644 --- a/src/parser/contexts.h +++ b/src/parser/contexts.h @@ -2800,8 +2800,8 @@ struct ParseDefsCtx : TypeParserCtx, AnnotationParserCtx { const std::vector& annotations, Index label, BrOnOp op, - Type in = Type::none, - Type out = Type::none) { + std::optional in = std::nullopt, + std::optional out = std::nullopt) { return withLoc( pos, irBuilder.makeBrOn(label, op, in, out, parseAnnotations(annotations))); diff --git a/src/wasm-ir-builder.h b/src/wasm-ir-builder.h index ab81ba549a7..5998dc87428 100644 --- a/src/wasm-ir-builder.h +++ b/src/wasm-ir-builder.h @@ -233,8 +233,8 @@ class IRBuilder : public UnifiedExpressionVisitor> { Result<> makeRefGetDesc(HeapType type); Result<> makeBrOn(Index label, BrOnOp op, - Type in = Type::none, - Type out = Type::none, + std::optional in = std::nullopt, + std::optional out = std::nullopt, const CodeAnnotation& annotations = {}); Result<> makeStructNew(HeapType type, bool isDesc); Result<> makeStructNewDefault(HeapType type, bool isDesc); diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 4e35d8ce0c3..913f050e835 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -23,6 +23,7 @@ #include "ir/properties.h" #include "ir/utils.h" #include "wasm-ir-builder.h" +#include "wasm.h" #ifndef IR_BUILDER_DEBUG #define IR_BUILDER_DEBUG 0 @@ -674,6 +675,14 @@ struct IRBuilder::ChildPopper return popConstrainedChildren(children); } + Result<> visitBrOn(BrOn* curr, + std::optional in = std::nullopt, + std::optional out = std::nullopt) { + std::vector children; + ConstraintCollector{builder, children}.visitBrOn(curr, in, out); + return popConstrainedChildren(children); + } + Result<> visitSwitch(Switch* curr, std::optional labelType = std::nullopt) { std::vector children; @@ -2087,13 +2096,16 @@ Result<> IRBuilder::makeRefGetDesc(HeapType type) { Result<> IRBuilder::makeBrOn(Index label, BrOnOp op, - Type in, - Type out, + std::optional in, + std::optional out, const CodeAnnotation& annotations) { + bool isCast = op != BrOnNull && op != BrOnNonNull; + bool isDescCast = op == BrOnCastDescEq || op == BrOnCastDescEqFail; + assert(bool(in) == bool(out) && bool(in) == isCast); std::optional descriptor; - if (op == BrOnCastDescEq || op == BrOnCastDescEqFail) { - assert(out.isRef()); - descriptor = out.getHeapType().getDescriptorType(); + if (isDescCast) { + assert(out && out->isRef()); + descriptor = out->getHeapType().getDescriptorType(); if (!descriptor) { return Err{"cast target must have descriptor"}; } @@ -2101,12 +2113,12 @@ Result<> IRBuilder::makeBrOn(Index label, BrOn curr; curr.op = op; - curr.castType = out; + curr.castType = isCast ? *out : Type::none; curr.desc = nullptr; - if (op != BrOnNull && op != BrOnNonNull && !out.isCastable()) { + if (isCast && !out->isCastable()) { return Err{"br_on cannot cast to invalid type"}; } - CHECK_ERR(visitBrOn(&curr)); + CHECK_ERR(ChildPopper{*this}.visitBrOn(&curr, in, out)); // Validate things that would cause errors later. if (curr.ref->type != Type::unreachable && !curr.ref->type.isRef()) { @@ -2124,14 +2136,14 @@ Result<> IRBuilder::makeBrOn(Index label, break; case BrOnCastDescEq: case BrOnCastDescEqFail: { - CHECK_ERR(validateTypeAnnotation(out.with(*descriptor).with(Nullable), + CHECK_ERR(validateTypeAnnotation(out->with(*descriptor).with(Nullable), curr.desc)); } [[fallthrough]]; case BrOnCast: case BrOnCastFail: - assert(in.isRef()); - CHECK_ERR(validateTypeAnnotation(in, curr.ref)); + assert(in->isRef()); + CHECK_ERR(validateTypeAnnotation(*in, curr.ref)); } // Extra values need to be sent in a scratch local. @@ -2168,7 +2180,7 @@ Result<> IRBuilder::makeBrOn(Index label, case BrOnCastFail: case BrOnCastDescEq: case BrOnCastDescEqFail: - testType = in; + testType = *in; break; } @@ -2181,7 +2193,7 @@ Result<> IRBuilder::makeBrOn(Index label, auto name = getLabelName(label); CHECK_ERR(name); - auto* br = builder.makeBrOn(op, *name, curr.ref, out, curr.desc); + auto* br = builder.makeBrOn(op, *name, curr.ref, curr.castType, curr.desc); applyAnnotations(br, annotations); push(br); return Ok{}; @@ -2205,7 +2217,8 @@ Result<> IRBuilder::makeBrOn(Index label, // Perform the branch. CHECK_ERR(visitBrOn(&curr)); - auto* br = builder.makeBrOn(op, extraLabel, curr.ref, out, curr.desc); + auto* br = + builder.makeBrOn(op, extraLabel, curr.ref, curr.castType, curr.desc); applyAnnotations(br, annotations); push(br); @@ -2227,18 +2240,18 @@ Result<> IRBuilder::makeBrOn(Index label, WASM_UNREACHABLE("unexpected op"); case BrOnCast: case BrOnCastDescEq: - if (out.isNullable()) { - resultType = Type(in.getHeapType(), NonNullable); + if (out->isNullable()) { + resultType = Type(in->getHeapType(), NonNullable); } else { - resultType = in; + resultType = *in; } break; case BrOnCastFail: case BrOnCastDescEqFail: - if (in.isNonNullable()) { - resultType = Type(out.getHeapType(), NonNullable); + if (in->isNonNullable()) { + resultType = Type(out->getHeapType(), NonNullable); } else { - resultType = out; + resultType = *out; } break; } diff --git a/test/lit/basic/unreachable-br-on-cast-desc.wast b/test/lit/basic/unreachable-br-on-cast-desc.wast new file mode 100644 index 00000000000..70613347acb --- /dev/null +++ b/test/lit/basic/unreachable-br-on-cast-desc.wast @@ -0,0 +1,41 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited. + +;; RUN: wasm-opt %s -all -S -o - | filecheck %s + +;; Regression test for a parser bug where we would incorrectly pop a +;; nullable `ref` operand for an unreachable br_on_cast_desc_eq{_fail} that +;; expected a non-nullable `ref`, then immediately fail the relevant type check. + +(module + (rec + ;; CHECK: (rec + ;; CHECK-NEXT: (type $s (descriptor $d) (struct)) + (type $s (descriptor $d) (struct)) + ;; CHECK: (type $d (describes $s) (struct)) + (type $d (describes $s) (struct)) + ) + ;; CHECK: (func $unreachable-br-on-cast-desc (type $2) (result (ref $s)) + ;; CHECK-NEXT: (block $label (result (ref $s)) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (ref.null none) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (block ;; (replaces unreachable BrOn we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $unreachable-br-on-cast-desc (result (ref $s)) + (br_on_cast_desc_eq 0 (ref $s) (ref $s) + ;; The input type is expected to be non-nullable, so this cannot be the + ;; input to the cast. We should create a new `unreachable` instead. + (ref.null none) + (unreachable) + ) + ) +)