From ff0e6fb9274d8110388863b5d67f88bf3ab90a95 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Sun, 4 Oct 2026 21:01:51 -0700 Subject: [PATCH] EffectAnalyzer: Do not reorder mayNotReturn with traps or global writes Expressions that may not return (loops, atomic.wait, struct.wait) can fail to terminate, which would prevent a subsequent trap or global state write (including calls) from ever executing. Update EffectAnalyzer::orderedBefore to disallow reordering mayNotReturn with traps or global writes, set mayNotReturn on AtomicWait, and clear loopEffects.mayNotReturn in LoopInvariantCodeMotion for the whole loop summary. Fixes #9182. --- src/ir/effects.h | 8 ++ src/passes/LoopInvariantCodeMotion.cpp | 7 ++ src/tools/fuzzing/fuzzing.cpp | 11 +++ test/gtest/effects.cpp | 90 +++++++++++++++++++ test/lit/passes/licm.wast | 89 ++++++++++++++++-- test/lit/passes/merge-blocks.wast | 87 +++++++++++++++++- .../passes/optimize-instructions-default.wast | 38 ++++++++ test/lit/passes/vacuum-tnh.wast | 46 ++++++++++ 8 files changed, 367 insertions(+), 9 deletions(-) diff --git a/src/ir/effects.h b/src/ir/effects.h index 24e50e5d28c..f3504fd4104 100644 --- a/src/ir/effects.h +++ b/src/ir/effects.h @@ -443,6 +443,12 @@ class EffectAnalyzer { return true; } } + // Cannot reorder code that may not return with traps or global state + // changes. + if ((mayNotReturn && (other.trap || other.writesGlobalState())) || + (other.mayNotReturn && (trap || writesGlobalState()))) { + return true; + } return false; } @@ -833,6 +839,8 @@ class EffectAnalyzer { parent.readOrder = parent.writeOrder = MemoryOrder::SeqCst; // Traps on unaligned accesses. parent.implicitTrap = true; + // If the timeout is negative and no-one wakes us. + parent.mayNotReturn = true; } void visitAtomicNotify(AtomicNotify* curr) { // Notifies on unshared memories just return 0 or trap on unaligned diff --git a/src/passes/LoopInvariantCodeMotion.cpp b/src/passes/LoopInvariantCodeMotion.cpp index c524b82ae9d..8fce0b9164c 100644 --- a/src/passes/LoopInvariantCodeMotion.cpp +++ b/src/passes/LoopInvariantCodeMotion.cpp @@ -73,6 +73,13 @@ struct LoopInvariantCodeMotion EffectAnalyzer loopEffects(getPassOptions(), *getModule(), loop); loopEffects.localsRead.clear(); loopEffects.localsWritten.clear(); + // We can ignore the loop's mayNotReturn effect because any hoisted + // instruction already executes on the first iteration before the loop can + // branch back (or before any inner loop after it; inner loops before it are + // tracked in effectsSoFar), and since the hoisted instruction is + // loop-invariant, it cannot trap on later iterations without trapping on + // the first. + loopEffects.mayNotReturn = false; // Note all the sets in each loop, and how many per index. Currently // EffectAnalyzer can't do that, and we need it to know if we // can move a set out of the loop (if there is another set diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 94e5c204df6..5ec13633afb 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -1887,6 +1887,17 @@ void TranslateToFuzzReader::modFunction(Function* func) { void TranslateToFuzzReader::addHangLimitChecks(Function* func) { // loop limit + // TODO: To detect bugs where optimizations erroneously reorder or eliminate + // infinite loops relative to traps or global effects (e.g. #9182), we + // could selectively omit hang limit instrumentation on pure, + // state-invariant loops. If an innermost loop has mayNotReturn but no + // traps, control flow transfers, global writes, allocations, or updates + // to locals read within the loop, its termination behavior is invariant + // across iterations (it either exits on iteration 1 or loops forever). + // Omitting instrumentation on such loops and relying on an interpreter- + // level iteration limit (with a distinct HangLimit result that skips + // native JS VM comparison) allows detecting mayNotReturn reordering + // bugs without false positives from altered iteration counts. for (auto* loop : FindAll(func->body).list) { loop->body = builder.makeSequence(makeHangLimitCheck(), loop->body, loop->type); diff --git a/test/gtest/effects.cpp b/test/gtest/effects.cpp index 4b8ee187e69..c0eb6f2dcf0 100644 --- a/test/gtest/effects.cpp +++ b/test/gtest/effects.cpp @@ -110,4 +110,94 @@ TEST_F(EffectAnalyzerTest, UnknownCall) { EXPECT_FALSE(effectsWithoutStackSwitch.suspends); } +TEST_F(EffectAnalyzerTest, MayNotReturnOrdering) { + auto moduleText = R"wasm( + (module + (memory 1 1 shared) + (global $g (mut i32) (i32.const 0)) + (func $callee) + (func $test (param $x i32) (param $y i32) + (loop $l1 + (br_if $l1 (local.get $x)) + ) + (loop $l2 + (br_if $l2 (local.get $y)) + ) + (drop (i32.div_s (i32.const 1) (local.get $y))) + (call $callee) + (global.set $g (i32.const 1)) + (local.set $y (i32.const 2)) + (drop (memory.atomic.wait32 (i32.const 0) (i32.const 0) (i64.const -1))) + ) + ) + )wasm"; + + auto parseResult = WATParser::parseModule(wasm, moduleText); + ASSERT_FALSE(parseResult.getErr()); + + auto* func = wasm.getFunction("test"); + ASSERT_NE(func, nullptr); + auto* block = func->body->cast(); + ASSERT_EQ(block->list.size(), 7u); + + auto* loop1 = block->list[0]; + auto* loop2 = block->list[1]; + auto* trapExpr = block->list[2]; + auto* callExpr = block->list[3]; + auto* globalSetExpr = block->list[4]; + auto* localSetExpr = block->list[5]; + auto* waitExpr = block->list[6]; + + EffectAnalyzer loop1Effects(options, wasm, loop1); + EffectAnalyzer loop2Effects(options, wasm, loop2); + EffectAnalyzer trapEffects(options, wasm, trapExpr); + EffectAnalyzer callEffects(options, wasm, callExpr); + EffectAnalyzer globalSetEffects(options, wasm, globalSetExpr); + EffectAnalyzer localSetEffects(options, wasm, localSetExpr); + EffectAnalyzer waitEffects(options, wasm, waitExpr); + + EXPECT_THAT(&loop1Effects, MayNotReturn()); + EXPECT_FALSE(loop1Effects.transfersControlFlow()); + EXPECT_THAT(&waitEffects, MayNotReturn()); + + // Cannot reorder mayNotReturn with traps, calls, or global writes. + EXPECT_TRUE(loop1Effects.orderedBefore(trapEffects)); + EXPECT_TRUE(trapEffects.orderedBefore(loop1Effects)); + EXPECT_FALSE(EffectAnalyzer::canReorder(options, wasm, loop1, trapExpr)); + EXPECT_FALSE(EffectAnalyzer::canReorder(options, wasm, trapExpr, loop1)); + + // Even with trapsNeverHappen, mayNotReturn (including atomic.wait) cannot be + // reordered with a trap. + PassOptions tnhOptions = options; + tnhOptions.trapsNeverHappen = true; + EXPECT_FALSE(EffectAnalyzer::canReorder(tnhOptions, wasm, loop1, trapExpr)); + EXPECT_FALSE(EffectAnalyzer::canReorder(tnhOptions, wasm, trapExpr, loop1)); + EXPECT_FALSE( + EffectAnalyzer::canReorder(tnhOptions, wasm, waitExpr, trapExpr)); + EXPECT_FALSE( + EffectAnalyzer::canReorder(tnhOptions, wasm, trapExpr, waitExpr)); + + EXPECT_TRUE(loop1Effects.orderedBefore(callEffects)); + EXPECT_TRUE(callEffects.orderedBefore(loop1Effects)); + EXPECT_FALSE(EffectAnalyzer::canReorder(options, wasm, loop1, callExpr)); + EXPECT_FALSE(EffectAnalyzer::canReorder(options, wasm, callExpr, loop1)); + + EXPECT_TRUE(loop1Effects.orderedBefore(globalSetEffects)); + EXPECT_TRUE(globalSetEffects.orderedBefore(loop1Effects)); + EXPECT_FALSE(EffectAnalyzer::canReorder(options, wasm, loop1, globalSetExpr)); + EXPECT_FALSE(EffectAnalyzer::canReorder(options, wasm, globalSetExpr, loop1)); + + // Can reorder mayNotReturn with unrelated local writes or another pure + // mayNotReturn. + EXPECT_FALSE(loop1Effects.orderedBefore(localSetEffects)); + EXPECT_FALSE(localSetEffects.orderedBefore(loop1Effects)); + EXPECT_TRUE(EffectAnalyzer::canReorder(options, wasm, loop1, localSetExpr)); + EXPECT_TRUE(EffectAnalyzer::canReorder(options, wasm, localSetExpr, loop1)); + + EXPECT_FALSE(loop1Effects.orderedBefore(loop2Effects)); + EXPECT_FALSE(loop2Effects.orderedBefore(loop1Effects)); + EXPECT_TRUE(EffectAnalyzer::canReorder(options, wasm, loop1, loop2)); + EXPECT_TRUE(EffectAnalyzer::canReorder(options, wasm, loop2, loop1)); +} + } // anonymous namespace diff --git a/test/lit/passes/licm.wast b/test/lit/passes/licm.wast index d1918c95491..89396d25ce3 100644 --- a/test/lit/passes/licm.wast +++ b/test/lit/passes/licm.wast @@ -5,9 +5,9 @@ (module (memory 10 20) - ;; CHECK: (type $0 (func (param i32))) + ;; CHECK: (type $0 (func (param i32) (result i32))) - ;; CHECK: (type $1 (func (param i32) (result i32))) + ;; CHECK: (type $1 (func (param i32))) ;; CHECK: (type $2 (func)) @@ -37,7 +37,7 @@ ) ) - ;; CHECK: (func $unreachable-get-call (type $0) (param $p i32) + ;; CHECK: (func $unreachable-get-call (type $1) (param $p i32) ;; CHECK-NEXT: (local $x i32) ;; CHECK-NEXT: (loop $loop ;; CHECK-NEXT: (unreachable) @@ -58,7 +58,7 @@ ) ) - ;; CHECK: (func $unreachable-get-store (type $0) (param $p i32) + ;; CHECK: (func $unreachable-get-store (type $1) (param $p i32) ;; CHECK-NEXT: (local $x i32) ;; CHECK-NEXT: (loop $loop ;; CHECK-NEXT: (unreachable) @@ -81,7 +81,7 @@ ) ) - ;; CHECK: (func $pause (type $0) (param $p i32) + ;; CHECK: (func $pause (type $1) (param $p i32) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) @@ -107,7 +107,7 @@ ) ) - ;; CHECK: (func $bug-inversion (type $1) (param $z i32) (result i32) + ;; CHECK: (func $bug-inversion (type $0) (param $z i32) (result i32) ;; CHECK-NEXT: (local $x i32) ;; CHECK-NEXT: (local $y i32) ;; CHECK-NEXT: (local.set $y @@ -145,7 +145,7 @@ (local.get $x) ) - ;; CHECK: (func $bug-cross-statement-dependency (type $1) (param $z i32) (result i32) + ;; CHECK: (func $bug-cross-statement-dependency (type $0) (param $z i32) (result i32) ;; CHECK-NEXT: (local $x i32) ;; CHECK-NEXT: (local $y i32) ;; CHECK-NEXT: (local.set $x @@ -182,4 +182,79 @@ ) (local.get $y) ) + + ;; CHECK: (func $hoist-trap-before-backedge (type $0) (param $x i32) (result i32) + ;; CHECK-NEXT: (local $y i32) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (local.set $y + ;; CHECK-NEXT: (i32.load + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (loop $loop + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (br_if $loop + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: ) + (func $hoist-trap-before-backedge (param $x i32) (result i32) + (local $y i32) + ;; A trapping instruction before the loop's own backedge can still be + ;; hoisted, because the first iteration always executes it. + (loop $loop + (local.set $y + (i32.load + (i32.const 0) + ) + ) + (br_if $loop + (local.get $x) + ) + ) + (local.get $y) + ) + + ;; CHECK: (func $no-hoist-trap-past-inner-loop (type $0) (param $x i32) (result i32) + ;; CHECK-NEXT: (local $y i32) + ;; CHECK-NEXT: (loop $outer + ;; CHECK-NEXT: (loop $inner + ;; CHECK-NEXT: (br_if $inner + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.set $y + ;; CHECK-NEXT: (i32.load + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (br_if $outer + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: ) + (func $no-hoist-trap-past-inner-loop (param $x i32) (result i32) + (local $y i32) + ;; A trapping instruction after an inner loop cannot be hoisted before the + ;; outer loop, because the inner loop might not terminate. + (loop $outer + (loop $inner + (br_if $inner + (local.get $x) + ) + ) + (local.set $y + (i32.load + (i32.const 0) + ) + ) + (br_if $outer + (local.get $x) + ) + ) + (local.get $y) + ) ) diff --git a/test/lit/passes/merge-blocks.wast b/test/lit/passes/merge-blocks.wast index 86181f7a488..06984224b02 100644 --- a/test/lit/passes/merge-blocks.wast +++ b/test/lit/passes/merge-blocks.wast @@ -186,7 +186,7 @@ ) ) - ;; CHECK: (func $if-condition (type $6) (result i32) + ;; CHECK: (func $if-condition (type $7) (result i32) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) @@ -424,7 +424,90 @@ ) ) - ;; CHECK: (func $helper (type $7) (param $x i32) (result i32) + ;; CHECK: (func $subsequent-children-loop-call (type $6) (param $x i32) (param $y i32) (result i32) + ;; CHECK-NEXT: (call $subsequent-children + ;; CHECK-NEXT: (loop $loop (result i32) + ;; CHECK-NEXT: (br_if $loop + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 2) + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (call $helper + ;; CHECK-NEXT: (i32.const 3) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 4) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $subsequent-children-loop-call (param $x i32) (param $y i32) (result i32) + ;; A loop that may not return in an earlier child prevents moving a call out + ;; of a later child past it. + (call $subsequent-children + (loop $loop (result i32) + (br_if $loop + (local.get $x) + ) + (i32.const 1) + ) + (i32.const 2) + (block (result i32) + (drop + (call $helper + (i32.const 3) + ) + ) + (i32.const 4) + ) + ) + ) + + ;; CHECK: (func $subsequent-children-loop-trap (type $6) (param $x i32) (param $y i32) (result i32) + ;; CHECK-NEXT: (i32.add + ;; CHECK-NEXT: (loop $loop (result i32) + ;; CHECK-NEXT: (br_if $loop + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.div_s + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 2) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $subsequent-children-loop-trap (param $x i32) (param $y i32) (result i32) + ;; A loop that may not return in an earlier child prevents moving a trap out + ;; of a later child past it. + (i32.add + (loop $loop (result i32) + (br_if $loop + (local.get $x) + ) + (i32.const 1) + ) + (block (result i32) + (drop + (i32.div_s + (i32.const 1) + (local.get $y) + ) + ) + (i32.const 2) + ) + ) + ) + + + ;; CHECK: (func $helper (type $8) (param $x i32) (result i32) ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) (func $helper (param $x i32) (result i32) diff --git a/test/lit/passes/optimize-instructions-default.wast b/test/lit/passes/optimize-instructions-default.wast index ab40b4dd5b6..437deac1bd2 100644 --- a/test/lit/passes/optimize-instructions-default.wast +++ b/test/lit/passes/optimize-instructions-default.wast @@ -127,4 +127,42 @@ (drop (i32.shr_s (i32.shl (local.get $x) (i32.const 16)) (i32.const 24))) ;; skip (drop (i32.shr_s (i32.shl (local.get $x) (i32.const 24)) (i32.const 16))) ;; skip ) + + ;; CHECK: (func $no-reorder-may-not-return (param $x i32) (param $y i32) (result i32) + ;; CHECK-NEXT: (i32.add + ;; CHECK-NEXT: (i32.sub + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (loop $loop (result i32) + ;; CHECK-NEXT: (br_if $loop + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.div_s + ;; CHECK-NEXT: (i32.const 3) + ;; CHECK-NEXT: (local.get $y) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $no-reorder-may-not-return (param $x i32) (param $y i32) (result i32) + ;; OptimizeInstructions normally rewrites (0 - X) + Y => Y - X when X and Y + ;; can be reordered. That rewrite cannot happen here because X is a loop + ;; that may not return and Y may trap. + (i32.add + (i32.sub + (i32.const 0) + (loop $loop (result i32) + (br_if $loop + (local.get $x) + ) + (i32.const 1) + ) + ) + (i32.div_s + (i32.const 3) + (local.get $y) + ) + ) + ) ) diff --git a/test/lit/passes/vacuum-tnh.wast b/test/lit/passes/vacuum-tnh.wast index 89d96a562df..98cfdf177a7 100644 --- a/test/lit/passes/vacuum-tnh.wast +++ b/test/lit/passes/vacuum-tnh.wast @@ -777,4 +777,50 @@ ) (unreachable) ) + + ;; YESTNH: (func $unreached-atomic-wait (type $0) + ;; YESTNH-NEXT: (i32.store + ;; YESTNH-NEXT: (i32.const 0) + ;; YESTNH-NEXT: (i32.const 1) + ;; YESTNH-NEXT: ) + ;; YESTNH-NEXT: (drop + ;; YESTNH-NEXT: (memory.atomic.wait32 + ;; YESTNH-NEXT: (i32.const 0) + ;; YESTNH-NEXT: (i32.const 0) + ;; YESTNH-NEXT: (i64.const -1) + ;; YESTNH-NEXT: ) + ;; YESTNH-NEXT: ) + ;; YESTNH-NEXT: (unreachable) + ;; YESTNH-NEXT: ) + ;; NO_TNH: (func $unreached-atomic-wait (type $0) + ;; NO_TNH-NEXT: (i32.store + ;; NO_TNH-NEXT: (i32.const 0) + ;; NO_TNH-NEXT: (i32.const 1) + ;; NO_TNH-NEXT: ) + ;; NO_TNH-NEXT: (drop + ;; NO_TNH-NEXT: (memory.atomic.wait32 + ;; NO_TNH-NEXT: (i32.const 0) + ;; NO_TNH-NEXT: (i32.const 0) + ;; NO_TNH-NEXT: (i64.const -1) + ;; NO_TNH-NEXT: ) + ;; NO_TNH-NEXT: ) + ;; NO_TNH-NEXT: (unreachable) + ;; NO_TNH-NEXT: ) + (func $unreached-atomic-wait + ;; Like an infinite loop, an atomic.wait may never return (if the timeout is + ;; negative and no one wakes it), so it and any preceding side effects + ;; cannot be removed in TNH mode even when followed by an unreachable. + (i32.store + (i32.const 0) + (i32.const 1) + ) + (drop + (memory.atomic.wait32 + (i32.const 0) + (i32.const 0) + (i64.const -1) + ) + ) + (unreachable) + ) )