From cb37815560ba59e1cb10e6532a41f0f9b8b3f38e Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Wed, 4 Mar 2026 17:57:43 -0800 Subject: [PATCH 1/5] Clean up null check handling in EffectAnalyzer Each visitor previously handled implicit null checks of reference operands in an ad hoc way. Different expressions had varying levels of precision when reasoning about the null checks. For example, many would just always set `implicitTraps`, while others would look at the nullability of the operands to be more precise. Factor the handling of null checks into a helper function and use it everywhere applicable. This makes the code smaller, clearer, and more precise. Update OptimizeCasts to add a new dummy operand to a dummy expression because EffectAnalyzer will newly try to look at that operand and it must not read uninitialized memory. Some GUFA tests are updated to remove more side-effect-free expressions now that EffectAnalyzer is more aggressive and can recognize them as such. --- src/ir/effects.h | 250 ++++++++++-------------- src/passes/OptimizeCasts.cpp | 5 + test/lit/passes/gufa-desc.wast | 102 ++-------- test/lit/passes/type-refining-gufa.wast | 14 +- 4 files changed, 127 insertions(+), 244 deletions(-) diff --git a/src/ir/effects.h b/src/ir/effects.h index 9f6590ffe72..754b4fc0bfe 100644 --- a/src/ir/effects.h +++ b/src/ir/effects.h @@ -20,6 +20,7 @@ #include "ir/intrinsics.h" #include "pass.h" #include "wasm-traversal.h" +#include "wasm.h" namespace wasm { @@ -509,6 +510,34 @@ class EffectAnalyzer { } } + // Handle effects due to an explicit null check of the operands in `exprs`. + // Returns true iff there is no need to consider further effects. + bool handleNullChecked(std::initializer_list exprs) { + for (auto* expr : exprs) { + if (expr && expr->type == Type::unreachable) { + return true; + } + } + for (auto* expr : exprs) { + assert(!expr || expr->type.isRef()); + if (expr && expr->type.isNull()) { + parent.trap = true; + return true; + } + } + for (auto* expr : exprs) { + if (expr && expr->type.isNullable()) { + parent.implicitTrap = true; + break; + } + } + return false; + } + + bool handleNullChecked(Expression* expr) { + return handleNullChecked({expr}); + } + void visitBlock(Block* curr) { if (curr->name.is()) { parent.breakTargets.erase(curr->name); // these were internal breaks @@ -823,11 +852,12 @@ class EffectAnalyzer { } } void visitThrowRef(ThrowRef* curr) { + if (handleNullChecked(curr->exnref)) { + return; + } if (parent.tryDepth == 0) { parent.throws_ = true; } - // traps when the arg is null - parent.implicitTrap = true; } void visitNop(Nop* curr) {} void visitUnreachable(Unreachable* curr) { parent.trap = true; } @@ -839,28 +869,17 @@ class EffectAnalyzer { void visitTupleMake(TupleMake* curr) {} void visitTupleExtract(TupleExtract* curr) {} void visitRefI31(RefI31* curr) {} - void visitI31Get(I31Get* curr) { - // traps when the ref is null - if (curr->i31->type.isNullable()) { - parent.implicitTrap = true; - } - } + void visitI31Get(I31Get* curr) { handleNullChecked(curr->i31); } void visitCallRef(CallRef* curr) { + if (handleNullChecked(curr->target)) { + return; + } if (curr->isReturn) { parent.branchesOut = true; if (parent.features.hasExceptionHandling()) { parent.hasReturnCallThrow = true; } } - if (curr->target->type.isNull()) { - parent.trap = true; - return; - } - // traps when the call target is null - if (curr->target->type.isNullable()) { - parent.implicitTrap = true; - } - parent.calls = true; if (parent.features.hasExceptionHandling() && (parent.tryDepth == 0 && !curr->isReturn)) { @@ -868,36 +887,24 @@ class EffectAnalyzer { } } void visitRefTest(RefTest* curr) {} - void maybeHandleDescriptor(Expression* desc) { - if (desc) { - // Traps when the descriptor is null. - if (desc->type.isNull()) { - parent.trap = true; - } else if (desc->type.isNullable()) { - parent.implicitTrap = true; - } - } - } + void visitRefCast(RefCast* curr) { + if (handleNullChecked(curr->desc)) { + return; + } // Traps if the cast fails. parent.implicitTrap = true; - maybeHandleDescriptor(curr->desc); - } - void visitRefGetDesc(RefGetDesc* curr) { - // Traps if the ref is null. - parent.implicitTrap = true; } + void visitRefGetDesc(RefGetDesc* curr) { handleNullChecked(curr->ref); } void visitBrOn(BrOn* curr) { + if (handleNullChecked(curr->desc)) { + return; + } parent.breakTargets.insert(curr->name); - maybeHandleDescriptor(curr->desc); } - void visitStructNew(StructNew* curr) { maybeHandleDescriptor(curr->desc); } + void visitStructNew(StructNew* curr) { handleNullChecked(curr->desc); } void visitStructGet(StructGet* curr) { - if (curr->ref->type == Type::unreachable) { - return; - } - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } if (curr->ref->type.getHeapType() @@ -906,10 +913,6 @@ class EffectAnalyzer { .mutable_ == Mutable) { parent.readsMutableStruct = true; } - // traps when the arg is null - if (curr->ref->type.isNullable()) { - parent.implicitTrap = true; - } switch (curr->order) { case MemoryOrder::Unordered: break; @@ -924,56 +927,38 @@ class EffectAnalyzer { } } void visitStructSet(StructSet* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.writesStruct = true; - // traps when the arg is null - if (curr->ref->type.isNullable()) { - parent.implicitTrap = true; - } if (curr->order != MemoryOrder::Unordered) { parent.isAtomic = true; } } void visitStructRMW(StructRMW* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.readsMutableStruct = true; parent.writesStruct = true; - if (curr->ref->type.isNullable()) { - parent.implicitTrap = true; - } assert(curr->order != MemoryOrder::Unordered); parent.isAtomic = true; } void visitStructCmpxchg(StructCmpxchg* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.readsMutableStruct = true; parent.writesStruct = true; - if (curr->ref->type.isNullable()) { - parent.implicitTrap = true; - } assert(curr->order != MemoryOrder::Unordered); parent.isAtomic = true; } void visitStructWait(StructWait* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.isAtomic = true; - if (curr->ref->type.isNullable()) { - parent.implicitTrap = true; - } - // If the timeout is negative and no-one wakes us. parent.mayNotReturn = true; @@ -982,10 +967,6 @@ class EffectAnalyzer { // isAtomic == true). parent.writesStruct = true; - if (curr->ref->type == Type::unreachable) { - return; - } - // If the ref isn't `unreachable`, then the field must exist and be a // packed waitqueue due to validation. assert(curr->ref->type.isStruct()); @@ -1002,16 +983,11 @@ class EffectAnalyzer { .mutable_ == Mutable; } void visitStructNotify(StructNotify* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.isAtomic = true; - if (curr->ref->type.isNullable()) { - parent.implicitTrap = true; - } - // struct.notify mutates an opaque waiter queue which isn't visible in // user code. Model this as a struct write which prevents reorderings // (since isAtomic == true). @@ -1030,8 +1006,7 @@ class EffectAnalyzer { } void visitArrayNewFixed(ArrayNewFixed* curr) {} void visitArrayGet(ArrayGet* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.readsArray = true; @@ -1039,8 +1014,7 @@ class EffectAnalyzer { parent.implicitTrap = true; } void visitArraySet(ArraySet* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.writesArray = true; @@ -1048,18 +1022,12 @@ class EffectAnalyzer { parent.implicitTrap = true; } void visitArrayLen(ArrayLen* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } - // traps when the arg is null - if (curr->ref->type.isNullable()) { - parent.implicitTrap = true; - } } void visitArrayCopy(ArrayCopy* curr) { - if (curr->destRef->type.isNull() || curr->srcRef->type.isNull()) { - parent.trap = true; + if (handleNullChecked({curr->srcRef, curr->destRef})) { return; } parent.readsArray = true; @@ -1068,8 +1036,7 @@ class EffectAnalyzer { parent.implicitTrap = true; } void visitArrayFill(ArrayFill* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.writesArray = true; @@ -1077,37 +1044,33 @@ class EffectAnalyzer { parent.implicitTrap = true; } template void visitArrayInit(ArrayInit* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.writesArray = true; - // Traps when the destination is null, when out of bounds in source or - // destination, or when the source segment has been dropped. + // OOB access to array or element segment. parent.implicitTrap = true; } void visitArrayInitData(ArrayInitData* curr) { visitArrayInit(curr); } void visitArrayInitElem(ArrayInitElem* curr) { visitArrayInit(curr); } void visitArrayRMW(ArrayRMW* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.readsArray = true; parent.writesArray = true; - // traps when the arg is null or the index out of bounds + // OOB access to array. parent.implicitTrap = true; assert(curr->order != MemoryOrder::Unordered); parent.isAtomic = true; } void visitArrayCmpxchg(ArrayCmpxchg* curr) { - if (curr->ref->type.isNull()) { - parent.trap = true; + if (handleNullChecked(curr->ref)) { return; } parent.readsArray = true; parent.writesArray = true; - // traps when the arg is null or the index out of bounds + // OOB access to array. parent.implicitTrap = true; assert(curr->order != MemoryOrder::Unordered); parent.isAtomic = true; @@ -1117,63 +1080,59 @@ class EffectAnalyzer { // These conversions are infallible. return; } - // traps when the arg is not valid - parent.implicitTrap = true; - // Note: We could be more precise here and report the lack of a possible - // trap if the input is non-nullable (and also of the right kind for - // RefAsFunc etc.). However, we have optimization passes that will - // remove a RefAs in such a case (in OptimizeInstructions, and also - // Vacuum in trapsNeverHappen mode), so duplicating that code here would - // only help until the next time those optimizations run. As a tradeoff, - // we keep the code here simpler, but it does mean another optimization - // cycle may be needed in some cases. + assert(curr->op == RefAsNonNull); + handleNullChecked(curr->value); } void visitStringNew(StringNew* curr) { - // traps when ref is null - parent.implicitTrap = true; - if (curr->op != StringNewFromCodePoint) { - parent.readsArray = true; + switch (curr->op) { + case StringNewLossyUTF8Array: + case StringNewWTF16Array: + if (handleNullChecked(curr->ref)) { + return; + } + parent.readsArray = true; + return; + case StringNewFromCodePoint: + return; } } void visitStringConst(StringConst* curr) {} void visitStringMeasure(StringMeasure* curr) { - // traps when ref is null. - parent.implicitTrap = true; + handleNullChecked(curr->ref); } void visitStringEncode(StringEncode* curr) { - // traps when ref is null or we write out of bounds. + if (handleNullChecked({curr->str, curr->array})) { + return; + } + // OOB array access. parent.implicitTrap = true; parent.writesArray = true; } void visitStringConcat(StringConcat* curr) { - // traps when an input is null. - parent.implicitTrap = true; + handleNullChecked({curr->left, curr->right}); } void visitStringEq(StringEq* curr) { - if (curr->op == StringEqCompare) { - // traps when either input is null. - if (curr->left->type.isNullable() || curr->right->type.isNullable()) { - parent.implicitTrap = true; - } + switch (curr->op) { + case StringEqEqual: + // Nulls are ok. + return; + case StringEqCompare: + handleNullChecked({curr->left, curr->right}); + return; } } void visitStringTest(StringTest* curr) {} void visitStringWTF16Get(StringWTF16Get* curr) { - // traps when ref is null. - parent.implicitTrap = true; + handleNullChecked(curr->ref); } void visitStringSliceWTF(StringSliceWTF* curr) { - // traps when ref is null. - parent.implicitTrap = true; - } - void visitContNew(ContNew* curr) { - // traps when curr->func is null ref. - parent.implicitTrap = true; + handleNullChecked(curr->ref); } + void visitContNew(ContNew* curr) { handleNullChecked(curr->func); } void visitContBind(ContBind* curr) { - // traps when curr->cont is null ref. - parent.implicitTrap = true; - + if (handleNullChecked(curr->cont)) { + return; + } // The input continuation is modified, as it will trap if resumed. This is // a globally-noticeable effect, which we model as a call for now, but we // could in theory use something more refined here (|modifiesContinuation| @@ -1192,37 +1151,36 @@ class EffectAnalyzer { parent.implicitTrap = true; } void visitResume(Resume* curr) { + if (handleNullChecked(curr->cont)) { + return; + } // This acts as a kitchen sink effect. parent.calls = true; - // resume instructions accept nullable continuation references and trap - // on null. - parent.implicitTrap = true; - if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) { parent.throws_ = true; } } void visitResumeThrow(ResumeThrow* curr) { + if (handleNullChecked(curr->cont)) { + return; + } + // This acts as a kitchen sink effect. parent.calls = true; - // resume_throw instructions accept nullable continuation - // references and trap on null. - parent.implicitTrap = true; - if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) { parent.throws_ = true; } } void visitStackSwitch(StackSwitch* curr) { + if (handleNullChecked(curr->cont)) { + return; + } + // This acts as a kitchen sink effect. parent.calls = true; - // switch instructions accept nullable continuation references - // and trap on null. - parent.implicitTrap = true; - if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) { parent.throws_ = true; } diff --git a/src/passes/OptimizeCasts.cpp b/src/passes/OptimizeCasts.cpp index aa7168f97c2..55f9a72899e 100644 --- a/src/passes/OptimizeCasts.cpp +++ b/src/passes/OptimizeCasts.cpp @@ -168,8 +168,13 @@ struct EarlyCastFinder // TODO: generalize this when we handle more than RefAsNonNull. RefCast dummyRefCast(module->allocator); dummyRefCast.desc = nullptr; + // Use an arbitrary nullable reference operand to get conservative + // ref.as_non_null effects. + LocalGet dummyRefAsOperand(module->allocator); + dummyRefAsOperand.type = Type(HeapType::any, Nullable); RefAs dummyRefAs(module->allocator); dummyRefAs.op = RefAsNonNull; + dummyRefAs.value = &dummyRefAsOperand; testRefCast.visit(&dummyRefCast); testRefAs.visit(&dummyRefAs); diff --git a/test/lit/passes/gufa-desc.wast b/test/lit/passes/gufa-desc.wast index 50903078792..c5a3face6c4 100644 --- a/test/lit/passes/gufa-desc.wast +++ b/test/lit/passes/gufa-desc.wast @@ -12,9 +12,9 @@ (type $desc (sub (describes $struct) (struct (field funcref)))) ) - ;; CHECK: (type $2 (func (result i32))) + ;; CHECK: (type $2 (func)) - ;; CHECK: (type $3 (func)) + ;; CHECK: (type $3 (func (result i32))) ;; CHECK: (global $desc (ref (exact $desc)) (struct.new $desc ;; CHECK-NEXT: (ref.func $func) @@ -42,38 +42,19 @@ ;; CHECK: (export "struct" (func $struct)) - ;; CHECK: (func $func (type $2) (result i32) + ;; CHECK: (func $func (type $3) (result i32) ;; CHECK-NEXT: (i32.const -1) ;; CHECK-NEXT: ) (func $func (result i32) (i32.const -1) ) - ;; CHECK: (func $desc (type $3) + ;; CHECK: (func $desc (type $2) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result (ref (exact $desc))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $struct - ;; CHECK-NEXT: (global.get $struct) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.get $desc) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.get $desc) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result (ref (exact $2))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result (ref (exact $desc))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $struct - ;; CHECK-NEXT: (global.get $struct) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.get $desc) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (ref.func $func) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (ref.func $func) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $desc (export "desc") @@ -93,7 +74,7 @@ ) ) - ;; CHECK: (func $struct (type $3) + ;; CHECK: (func $struct (type $2) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 100) ;; CHECK-NEXT: ) @@ -214,14 +195,7 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result (ref (exact $6))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $struct - ;; CHECK-NEXT: (global.get $struct) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (ref.func $func) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (ref.func $func) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $desc (export "desc") @@ -288,14 +262,7 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result (ref (exact $subdesc))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $sub - ;; CHECK-NEXT: (local.get $temp) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.get $subdesc) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.get $subdesc) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 100) @@ -452,10 +419,8 @@ ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result (ref (exact $desc))) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $struct - ;; CHECK-NEXT: (ref.cast (ref (exact $struct)) - ;; CHECK-NEXT: (global.get $struct) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (ref.cast (ref (exact $struct)) + ;; CHECK-NEXT: (global.get $struct) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (global.get $desc) @@ -473,10 +438,8 @@ ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result (ref (exact $desc))) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $struct - ;; CHECK-NEXT: (ref.cast (ref (exact $struct)) - ;; CHECK-NEXT: (global.get $struct) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (ref.cast (ref (exact $struct)) + ;; CHECK-NEXT: (global.get $struct) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (global.get $desc) @@ -576,46 +539,13 @@ ;; CHECK: (func $test (type $3) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result (ref (exact $B))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $A - ;; CHECK-NEXT: (global.get $A) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.get $B) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.get $B) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result (ref (exact $C))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $B - ;; CHECK-NEXT: (global.get $B) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.get $C) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.get $C) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result i32) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result (ref (exact $C))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $B - ;; CHECK-NEXT: (block (result (ref (exact $B))) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (ref.get_desc $A - ;; CHECK-NEXT: (global.get $A) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.get $B) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (global.get $C) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (i32.const 10) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 10) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $test (export "test") diff --git a/test/lit/passes/type-refining-gufa.wast b/test/lit/passes/type-refining-gufa.wast index a33b70c29b4..d6330890d8f 100644 --- a/test/lit/passes/type-refining-gufa.wast +++ b/test/lit/passes/type-refining-gufa.wast @@ -524,8 +524,6 @@ ;; GUFA: (export "a" (func $a)) ;; O3O3: (type $3 (func (result (ref $cont)))) - ;; O3O3: (elem declare func $ref) - ;; O3O3: (export "a" (func $a)) (export "a" (func $a)) ;; NRML: (export "b" (func $b)) @@ -556,15 +554,10 @@ ;; GUFA-NEXT: ) ;; GUFA-NEXT: ) ;; O3O3: (func $a (type $func) - ;; O3O3-NEXT: (drop - ;; O3O3-NEXT: (cont.new $cont - ;; O3O3-NEXT: (ref.func $ref) - ;; O3O3-NEXT: ) - ;; O3O3-NEXT: ) + ;; O3O3-NEXT: (nop) ;; O3O3-NEXT: ) (func $a - ;; GUFA cannot improve things here (-O3 can remove the struct operations, - ;; though). + ;; GUFA cannot improve things here (-O3 can remove everything, though). (drop (struct.get $wrap-cont 0 (struct.new $wrap-cont @@ -607,9 +600,6 @@ ;; GUFA-NEXT: ) ;; GUFA-NEXT: ) ;; GUFA-NEXT: ) - ;; O3O3: (func $ref (type $func) - ;; O3O3-NEXT: (nop) - ;; O3O3-NEXT: ) (func $ref (type $func) (drop (struct.new $wrap-array From b49ee65ec1341dddab4fcc7e56340d2935743584 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Thu, 5 Mar 2026 09:47:15 -0800 Subject: [PATCH 2/5] Fix OOB string access effects --- src/ir/effects.h | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/ir/effects.h b/src/ir/effects.h index 754b4fc0bfe..d100bfa49ad 100644 --- a/src/ir/effects.h +++ b/src/ir/effects.h @@ -1123,10 +1123,18 @@ class EffectAnalyzer { } void visitStringTest(StringTest* curr) {} void visitStringWTF16Get(StringWTF16Get* curr) { - handleNullChecked(curr->ref); + if (handleNullChecked(curr->ref)) { + return; + } + // OOB string access. + parent.implicitTrap = true; } void visitStringSliceWTF(StringSliceWTF* curr) { - handleNullChecked(curr->ref); + if (handleNullChecked(curr->ref)) { + return; + } + // OOB string access. + parent.implicitTrap = true; } void visitContNew(ContNew* curr) { handleNullChecked(curr->func); } void visitContBind(ContBind* curr) { From feb6a5859bbbcb80147a97f740525999c10f2f5b Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Thu, 5 Mar 2026 12:04:37 -0800 Subject: [PATCH 3/5] another array OOB --- src/ir/effects.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ir/effects.h b/src/ir/effects.h index 9f098c1be6d..5e6943180a1 100644 --- a/src/ir/effects.h +++ b/src/ir/effects.h @@ -1090,6 +1090,8 @@ class EffectAnalyzer { if (handleNullChecked(curr->ref)) { return; } + // OOB acces to array. + parent.implicitTrap = true; parent.readsArray = true; return; case StringNewFromCodePoint: From e78d2d93c2f7f1a3dd0eb06b9fd94468fdb0029f Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Thu, 5 Mar 2026 12:05:48 -0800 Subject: [PATCH 4/5] trapOnNull --- src/ir/effects.h | 76 +++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 40 deletions(-) diff --git a/src/ir/effects.h b/src/ir/effects.h index 5e6943180a1..2975e5b9e64 100644 --- a/src/ir/effects.h +++ b/src/ir/effects.h @@ -515,7 +515,7 @@ class EffectAnalyzer { // Handle effects due to an explicit null check of the operands in `exprs`. // Returns true iff there is no need to consider further effects. - bool handleNullChecked(std::initializer_list exprs) { + bool trapOnNull(std::initializer_list exprs) { for (auto* expr : exprs) { if (expr && expr->type == Type::unreachable) { return true; @@ -537,9 +537,7 @@ class EffectAnalyzer { return false; } - bool handleNullChecked(Expression* expr) { - return handleNullChecked({expr}); - } + bool trapOnNull(Expression* expr) { return trapOnNull({expr}); } void visitBlock(Block* curr) { if (curr->name.is()) { @@ -859,7 +857,7 @@ class EffectAnalyzer { } } void visitThrowRef(ThrowRef* curr) { - if (handleNullChecked(curr->exnref)) { + if (trapOnNull(curr->exnref)) { return; } if (parent.tryDepth == 0) { @@ -876,9 +874,9 @@ class EffectAnalyzer { void visitTupleMake(TupleMake* curr) {} void visitTupleExtract(TupleExtract* curr) {} void visitRefI31(RefI31* curr) {} - void visitI31Get(I31Get* curr) { handleNullChecked(curr->i31); } + void visitI31Get(I31Get* curr) { trapOnNull(curr->i31); } void visitCallRef(CallRef* curr) { - if (handleNullChecked(curr->target)) { + if (trapOnNull(curr->target)) { return; } if (curr->isReturn) { @@ -896,22 +894,22 @@ class EffectAnalyzer { void visitRefTest(RefTest* curr) {} void visitRefCast(RefCast* curr) { - if (handleNullChecked(curr->desc)) { + if (trapOnNull(curr->desc)) { return; } // Traps if the cast fails. parent.implicitTrap = true; } - void visitRefGetDesc(RefGetDesc* curr) { handleNullChecked(curr->ref); } + void visitRefGetDesc(RefGetDesc* curr) { trapOnNull(curr->ref); } void visitBrOn(BrOn* curr) { - if (handleNullChecked(curr->desc)) { + if (trapOnNull(curr->desc)) { return; } parent.breakTargets.insert(curr->name); } - void visitStructNew(StructNew* curr) { handleNullChecked(curr->desc); } + void visitStructNew(StructNew* curr) { trapOnNull(curr->desc); } void visitStructGet(StructGet* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } if (curr->ref->type.getHeapType() @@ -924,7 +922,7 @@ class EffectAnalyzer { curr->isAtomic() && curr->ref->type.getHeapType().isShared(); } void visitStructSet(StructSet* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.writesStruct = true; @@ -932,7 +930,7 @@ class EffectAnalyzer { curr->isAtomic() && curr->ref->type.getHeapType().isShared(); } void visitStructRMW(StructRMW* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.readsMutableStruct = true; @@ -941,7 +939,7 @@ class EffectAnalyzer { parent.isAtomic = true; } void visitStructCmpxchg(StructCmpxchg* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.readsMutableStruct = true; @@ -950,7 +948,7 @@ class EffectAnalyzer { parent.isAtomic = true; } void visitStructWait(StructWait* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.isAtomic = true; @@ -979,7 +977,7 @@ class EffectAnalyzer { .mutable_ == Mutable; } void visitStructNotify(StructNotify* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.isAtomic = true; @@ -1002,7 +1000,7 @@ class EffectAnalyzer { } void visitArrayNewFixed(ArrayNewFixed* curr) {} void visitArrayGet(ArrayGet* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.readsArray = true; @@ -1012,7 +1010,7 @@ class EffectAnalyzer { curr->isAtomic() && curr->ref->type.getHeapType().isShared(); } void visitArraySet(ArraySet* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.writesArray = true; @@ -1022,12 +1020,12 @@ class EffectAnalyzer { curr->isAtomic() && curr->ref->type.getHeapType().isShared(); } void visitArrayLen(ArrayLen* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } } void visitArrayCopy(ArrayCopy* curr) { - if (handleNullChecked({curr->srcRef, curr->destRef})) { + if (trapOnNull({curr->srcRef, curr->destRef})) { return; } parent.readsArray = true; @@ -1036,7 +1034,7 @@ class EffectAnalyzer { parent.implicitTrap = true; } void visitArrayFill(ArrayFill* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.writesArray = true; @@ -1044,7 +1042,7 @@ class EffectAnalyzer { parent.implicitTrap = true; } template void visitArrayInit(ArrayInit* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.writesArray = true; @@ -1054,7 +1052,7 @@ class EffectAnalyzer { void visitArrayInitData(ArrayInitData* curr) { visitArrayInit(curr); } void visitArrayInitElem(ArrayInitElem* curr) { visitArrayInit(curr); } void visitArrayRMW(ArrayRMW* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.readsArray = true; @@ -1065,7 +1063,7 @@ class EffectAnalyzer { parent.isAtomic = true; } void visitArrayCmpxchg(ArrayCmpxchg* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } parent.readsArray = true; @@ -1081,13 +1079,13 @@ class EffectAnalyzer { return; } assert(curr->op == RefAsNonNull); - handleNullChecked(curr->value); + trapOnNull(curr->value); } void visitStringNew(StringNew* curr) { switch (curr->op) { case StringNewLossyUTF8Array: case StringNewWTF16Array: - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } // OOB acces to array. @@ -1099,11 +1097,9 @@ class EffectAnalyzer { } } void visitStringConst(StringConst* curr) {} - void visitStringMeasure(StringMeasure* curr) { - handleNullChecked(curr->ref); - } + void visitStringMeasure(StringMeasure* curr) { trapOnNull(curr->ref); } void visitStringEncode(StringEncode* curr) { - if (handleNullChecked({curr->str, curr->array})) { + if (trapOnNull({curr->str, curr->array})) { return; } // OOB array access. @@ -1111,7 +1107,7 @@ class EffectAnalyzer { parent.writesArray = true; } void visitStringConcat(StringConcat* curr) { - handleNullChecked({curr->left, curr->right}); + trapOnNull({curr->left, curr->right}); } void visitStringEq(StringEq* curr) { switch (curr->op) { @@ -1119,28 +1115,28 @@ class EffectAnalyzer { // Nulls are ok. return; case StringEqCompare: - handleNullChecked({curr->left, curr->right}); + trapOnNull({curr->left, curr->right}); return; } } void visitStringTest(StringTest* curr) {} void visitStringWTF16Get(StringWTF16Get* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } // OOB string access. parent.implicitTrap = true; } void visitStringSliceWTF(StringSliceWTF* curr) { - if (handleNullChecked(curr->ref)) { + if (trapOnNull(curr->ref)) { return; } // OOB string access. parent.implicitTrap = true; } - void visitContNew(ContNew* curr) { handleNullChecked(curr->func); } + void visitContNew(ContNew* curr) { trapOnNull(curr->func); } void visitContBind(ContBind* curr) { - if (handleNullChecked(curr->cont)) { + if (trapOnNull(curr->cont)) { return; } // The input continuation is modified, as it will trap if resumed. This is @@ -1161,7 +1157,7 @@ class EffectAnalyzer { parent.implicitTrap = true; } void visitResume(Resume* curr) { - if (handleNullChecked(curr->cont)) { + if (trapOnNull(curr->cont)) { return; } // This acts as a kitchen sink effect. @@ -1172,7 +1168,7 @@ class EffectAnalyzer { } } void visitResumeThrow(ResumeThrow* curr) { - if (handleNullChecked(curr->cont)) { + if (trapOnNull(curr->cont)) { return; } @@ -1184,7 +1180,7 @@ class EffectAnalyzer { } } void visitStackSwitch(StackSwitch* curr) { - if (handleNullChecked(curr->cont)) { + if (trapOnNull(curr->cont)) { return; } From fd38e89973b0c452b19858fe07b55700fdf74b24 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Thu, 5 Mar 2026 17:15:01 -0800 Subject: [PATCH 5/5] typo and invalid code points --- src/ir/effects.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ir/effects.h b/src/ir/effects.h index 2975e5b9e64..19a5615c391 100644 --- a/src/ir/effects.h +++ b/src/ir/effects.h @@ -1088,11 +1088,13 @@ class EffectAnalyzer { if (trapOnNull(curr->ref)) { return; } - // OOB acces to array. + // OOB access to array. parent.implicitTrap = true; parent.readsArray = true; return; case StringNewFromCodePoint: + // Invalid code points. + parent.implicitTrap = true; return; } }