From 951a4a8364d4c357f33c0b4e0d585dc3d0e918d2 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Sun, 18 Oct 2026 20:05:28 -0700 Subject: [PATCH 1/3] [StackIR] Emit unreachable for unwritable insts BinaryInstWriter emits an unreachable instruction when an expression has an unwritable heap type immediate or is a StringNew or StringEncode with a null-typed array operand, but StackIRGenerator::emit previously emitted a normal Basic StackInst for those expressions. Include StringNew and StringEncode with null-typed array operands in Properties::hasUnwritableTypeImmediate and emit unreachable in StackIRGenerator::emit when it returns true so StackIROptimizer and PrintStackIR see the unreachable instruction that will be written to the binary. Also fix BinaryInstWriter::visitStringEncode to check whether curr->array (rather than curr->str) has a null type. --- src/ir/properties.h | 12 +++- src/wasm/wasm-stack.cpp | 11 +++- test/lit/basic/unreachable-string-new.wast | 26 ++++++-- test/lit/passes/optimize-stack-ir.wast | 75 ++++++++++++++++++++-- test/lit/strings.wast | 44 +++++++++++++ 5 files changed, 155 insertions(+), 13 deletions(-) diff --git a/src/ir/properties.h b/src/ir/properties.h index 59c35cf9cae..a06697a2371 100644 --- a/src/ir/properties.h +++ b/src/ir/properties.h @@ -530,8 +530,18 @@ inline MemoryOrder getMemoryOrder(Expression* curr) { // Whether this instruction will be unwritable in the text and binary formats // because it requires a type index immediate computed from an expression with -// unreachable or null type, and therefore no type index. +// unreachable or null type, and therefore no type index (or is a string array +// operation with a null array operand). inline bool hasUnwritableTypeImmediate(Expression* curr) { + if (auto* sn = curr->dynCast()) { + // See BinaryInstWriter::visitStringNew. + return sn->ref->type.isNull(); + } + if (auto* se = curr->dynCast()) { + // See BinaryInstWriter::visitStringEncode. + return se->array->type.isNull(); + } + #define DELEGATE_ID curr->_id #define DELEGATE_FIELD_IMMEDIATE_TYPED_CHILD(id, field) \ diff --git a/src/wasm/wasm-stack.cpp b/src/wasm/wasm-stack.cpp index e31f979d321..fbf68d00a0e 100644 --- a/src/wasm/wasm-stack.cpp +++ b/src/wasm/wasm-stack.cpp @@ -3032,7 +3032,7 @@ void BinaryInstWriter::visitStringMeasure(StringMeasure* curr) { } void BinaryInstWriter::visitStringEncode(StringEncode* curr) { - if (curr->str->type.isNull()) { + if (curr->array->type.isNull()) { // See visitStringNew. emitUnreachable(); return; @@ -3642,6 +3642,15 @@ class StackIRGenerator : public BinaryenIRWriter { }; void StackIRGenerator::emit(Expression* curr) { + // Match BinaryInstWriter (and PrintSExpression::visitExpression), which emits + // unreachable instead of instructions whose type immediate cannot be written + // (see BinaryInstWriter::visitCallRef, visitRefCast, visitStructGet, etc.) or + // string array operations with null-typed array operands (see + // BinaryInstWriter::visitStringNew and visitStringEncode). + if (Properties::hasUnwritableTypeImmediate(curr)) { + emitUnreachable(); + return; + } StackInst* stackInst = nullptr; if (curr->is()) { stackInst = makeStackInst(StackInst::BlockBegin, curr); diff --git a/test/lit/basic/unreachable-string-new.wast b/test/lit/basic/unreachable-string-new.wast index 91dd9d28930..2e6a3d3f30d 100644 --- a/test/lit/basic/unreachable-string-new.wast +++ b/test/lit/basic/unreachable-string-new.wast @@ -84,11 +84,18 @@ ) ;; CHECK: (func $i8-ok-none (type $4) (result (ref string)) - ;; CHECK-NEXT: (string.new_lossy_utf8_array - ;; CHECK-NEXT: (block (result (ref none)) + ;; CHECK-NEXT: (block ;; (replaces unreachable StringNew we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block (result (ref none)) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -219,11 +226,18 @@ ) ;; CHECK: (func $i16-ok-none (type $4) (result (ref string)) - ;; CHECK-NEXT: (string.new_wtf16_array - ;; CHECK-NEXT: (block (result (ref none)) + ;; CHECK-NEXT: (block ;; (replaces unreachable StringNew we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block (result (ref none)) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) diff --git a/test/lit/passes/optimize-stack-ir.wast b/test/lit/passes/optimize-stack-ir.wast index 5c3b591062f..0121312b2b5 100644 --- a/test/lit/passes/optimize-stack-ir.wast +++ b/test/lit/passes/optimize-stack-ir.wast @@ -16,6 +16,8 @@ ;; CHECK: (type $FUNCSIG$ddd (func (param f64 f64) (result f64))) + ;; CHECK: (type $8 (func (result i64))) + ;; CHECK: (type $10 (func (param f32) (result f32))) ;; CHECK: (type $FUNCSIG$id (func (param f64) (result i32))) @@ -26,14 +28,15 @@ (type $6 (func (param i32) (result i32))) ;; CHECK: (type $7 (func (param f64) (result f64))) (type $7 (func (param f64) (result f64))) - ;; CHECK: (type $8 (func (result i64))) (type $8 (func (result i64))) ;; CHECK: (type $9 (func (param i32 i64))) (type $9 (func (param i32 i64))) - ;; CHECK: (type $15 (func (param eqref) (result i32))) + ;; CHECK: (type $14 (func (param eqref) (result i32))) + + ;; CHECK: (type $15 (func (param eqref) (result eqref))) - ;; CHECK: (type $16 (func (param eqref) (result eqref))) + ;; CHECK: (type $16 (func (param stringref) (result i64))) ;; CHECK: (import "env" "_emscripten_asm_const_vi" (func $_emscripten_asm_const_vi (type $FUNCSIG$v))) (import "env" "_emscripten_asm_const_vi" (func $_emscripten_asm_const_vi)) @@ -1485,7 +1488,7 @@ ) ;; A ref.eq with an intervening value: its operands can be swapped. - ;; CHECK: (func $local-to-stack-ref-eq (type $15) (param $x eqref) (result i32) + ;; CHECK: (func $local-to-stack-ref-eq (type $14) (param $x eqref) (result i32) ;; CHECK-NEXT: (local $temp eqref) ;; CHECK-NEXT: local.get $x ;; CHECK-NEXT: call $eqref-value @@ -1505,7 +1508,7 @@ ) ) - ;; CHECK: (func $eqref-value (type $16) (param $x eqref) (result eqref) + ;; CHECK: (func $eqref-value (type $15) (param $x eqref) (result eqref) ;; CHECK-NEXT: local.get $x ;; CHECK-NEXT: call $eqref-value ;; CHECK-NEXT: ) @@ -1571,4 +1574,66 @@ ) ) ) + + ;; CHECK: (func $null-string-new (type $8) (result i64) + ;; CHECK-NEXT: (local $y i64) + ;; CHECK-NEXT: i64.const 1 + ;; CHECK-NEXT: local.set $y + ;; CHECK-NEXT: ref.null none + ;; CHECK-NEXT: ref.as_non_null + ;; CHECK-NEXT: i32.const 0 + ;; CHECK-NEXT: i32.const 0 + ;; CHECK-NEXT: unreachable + ;; CHECK-NEXT: ) + (func $null-string-new (result i64) + (local $y i64) + ;; Previously, local2Stack thought the un-DCE'd drop and + ;; string.new_wtf16_array below consumed all three operands and returned the + ;; stack to this height, so it wrongly eliminated this local.set and the + ;; final local.get $y. + (local.set $y + (i64.const 1) + ) + (drop + ;; Has a null-typed array ref, so the binary writer emits unreachable + ;; instead of consuming its three operands. StackIRGenerator must also + ;; emit unreachable so dce removes the drop and local.get $y below. + (string.new_wtf16_array + (ref.as_non_null + (ref.null none) + ) + (i32.const 0) + (i32.const 0) + ) + ) + (local.get $y) + ) + + ;; CHECK: (func $null-string-encode (type $16) (param $str stringref) (result i64) + ;; CHECK-NEXT: (local $y i64) + ;; CHECK-NEXT: i64.const 1 + ;; CHECK-NEXT: local.set $y + ;; CHECK-NEXT: local.get $str + ;; CHECK-NEXT: ref.null none + ;; CHECK-NEXT: ref.as_non_null + ;; CHECK-NEXT: i32.const 0 + ;; CHECK-NEXT: unreachable + ;; CHECK-NEXT: ) + (func $null-string-encode (param $str stringref) (result i64) + (local $y i64) + (local.set $y + (i64.const 1) + ) + (drop + ;; Same as above, for string.encode_wtf16_array with a null-typed array. + (string.encode_wtf16_array + (local.get $str) + (ref.as_non_null + (ref.null none) + ) + (i32.const 0) + ) + ) + (local.get $y) + ) ) diff --git a/test/lit/strings.wast b/test/lit/strings.wast index 63473b0b1a6..f900e1f930c 100644 --- a/test/lit/strings.wast +++ b/test/lit/strings.wast @@ -31,6 +31,8 @@ ;; CHECK: (type $8 (func (param stringref (ref $array) (ref $array16)))) + ;; CHECK: (type $9 (func (param stringref i32) (result i32))) + ;; CHECK: (import "env" "get-string-ref" (func $get-string-ref (type $4) (result externref))) (import "env" "get-string-ref" (func $get-string-ref (result externref))) @@ -202,6 +204,13 @@ ;; CHECK-NEXT: (i32.const 10) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (string.encode_wtf16_array + ;; CHECK-NEXT: (ref.null noextern) + ;; CHECK-NEXT: (local.get $array16) + ;; CHECK-NEXT: (i32.const 30) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $string.encode.gc (param $ref stringref) (param $array (ref $array)) (param $array16 (ref $array16)) (drop @@ -220,6 +229,41 @@ (i32.const 10) ) ) + ;; A null string reference is still a valid string.encode input and should + ;; not be replaced with unreachable. + (drop + (string.encode_wtf16_array + (ref.null noextern) + (local.get $array16) + (i32.const 30) + ) + ) + ) + + ;; CHECK: (func $string.encode.null-array (type $9) (param $ref stringref) (param $start i32) (result i32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $ref) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (ref.as_non_null + ;; CHECK-NEXT: (ref.null none) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $start) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + (func $string.encode.null-array (param $ref stringref) (param $start i32) (result i32) + ;; A null array reference has no array type and is replaced with + ;; unreachable. + (string.encode_wtf16_array + (local.get $ref) + (ref.as_non_null + (ref.null none) + ) + (local.get $start) + ) ) ;; CHECK: (func $string.from_code_point (type $3) From 972d469b11d20fd0902830f450853967e2dc876d Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Mon, 19 Oct 2026 21:35:13 -0700 Subject: [PATCH 2/3] Apply suggestion from @gemini-code-assist[bot] Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- src/ir/properties.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ir/properties.h b/src/ir/properties.h index a06697a2371..0db9b4bb1a0 100644 --- a/src/ir/properties.h +++ b/src/ir/properties.h @@ -535,11 +535,11 @@ inline MemoryOrder getMemoryOrder(Expression* curr) { inline bool hasUnwritableTypeImmediate(Expression* curr) { if (auto* sn = curr->dynCast()) { // See BinaryInstWriter::visitStringNew. - return sn->ref->type.isNull(); + return sn->ref && sn->ref->type.isNull(); } if (auto* se = curr->dynCast()) { // See BinaryInstWriter::visitStringEncode. - return se->array->type.isNull(); + return se->array && se->array->type.isNull(); } #define DELEGATE_ID curr->_id From 7167e6976e6be7ef8e3f976eec41ec039a30df38 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Tue, 20 Oct 2026 17:14:34 -0700 Subject: [PATCH 3/3] Revert "Apply suggestion from @gemini-code-assist[bot]" This reverts commit 972d469b11d20fd0902830f450853967e2dc876d. --- src/ir/properties.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ir/properties.h b/src/ir/properties.h index 0db9b4bb1a0..a06697a2371 100644 --- a/src/ir/properties.h +++ b/src/ir/properties.h @@ -535,11 +535,11 @@ inline MemoryOrder getMemoryOrder(Expression* curr) { inline bool hasUnwritableTypeImmediate(Expression* curr) { if (auto* sn = curr->dynCast()) { // See BinaryInstWriter::visitStringNew. - return sn->ref && sn->ref->type.isNull(); + return sn->ref->type.isNull(); } if (auto* se = curr->dynCast()) { // See BinaryInstWriter::visitStringEncode. - return se->array && se->array->type.isNull(); + return se->array->type.isNull(); } #define DELEGATE_ID curr->_id