From d27e023391015749ee5df823becb5df1987e7dcc Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 13 Apr 2023 16:35:17 -0700 Subject: [PATCH 1/6] Fix #5599 --- src/ir/LocalStructuralDominance.cpp | 6 ++++++ test/lit/passes/precompute-gc.wast | 30 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/ir/LocalStructuralDominance.cpp b/src/ir/LocalStructuralDominance.cpp index cfb75e006ed..bfd9075edd5 100644 --- a/src/ir/LocalStructuralDominance.cpp +++ b/src/ir/LocalStructuralDominance.cpp @@ -103,6 +103,12 @@ LocalStructuralDominance::LocalStructuralDominance(Function* func, } static void doLocalSet(Scanner* self, Expression** currp) { + auto* curr = *currp; + if (curr->type == Type::unreachable) { + // Unreachable sets are not emitted in the binary format, so we do not + // count them. + return; + } auto index = (*currp)->cast()->index; if (!self->localsSet[index]) { // This local is now set until the end of this scope. diff --git a/test/lit/passes/precompute-gc.wast b/test/lit/passes/precompute-gc.wast index 926c412b372..5550eba1f0f 100644 --- a/test/lit/passes/precompute-gc.wast +++ b/test/lit/passes/precompute-gc.wast @@ -1033,4 +1033,34 @@ ) ) ) + + ;; CHECK: (func $unreachable-set (type $none_=>_none) + ;; CHECK-NEXT: (local $x funcref) + ;; CHECK-NEXT: (local.tee $x + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (ref.as_non_null + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $unreachable-set + ;; An unreachable set is not helpful for validation of non-nullable locals + ;; since we won't emit it in the binary format. We ignore the set in that + ;; computation, and as a result we'll make the local nullable here after + ;; this pass runs and the set becomes unreachable (due to calling refinalize + ;; which propagates the unreachability). + (local $x (ref func)) + (local.set $x + (block (result (ref func)) + (unreachable) + ) + ) + (drop + (local.get $x) + ) + ) ) From f93fa615d64db47afe41d8288ede4af2b5be04d3 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 13 Apr 2023 16:39:05 -0700 Subject: [PATCH 2/6] use --- src/ir/LocalStructuralDominance.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ir/LocalStructuralDominance.cpp b/src/ir/LocalStructuralDominance.cpp index bfd9075edd5..9355046a84b 100644 --- a/src/ir/LocalStructuralDominance.cpp +++ b/src/ir/LocalStructuralDominance.cpp @@ -109,7 +109,7 @@ LocalStructuralDominance::LocalStructuralDominance(Function* func, // count them. return; } - auto index = (*currp)->cast()->index; + auto index = curr->cast()->index; if (!self->localsSet[index]) { // This local is now set until the end of this scope. self->localsSet[index] = true; From 5aa15f8337aa9b65482b8c4577b8094be9bc1735 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 18 Apr 2023 08:25:25 -0700 Subject: [PATCH 3/6] idea --- src/ir/ReFinalize.cpp | 21 +++++++++++++++- src/ir/utils.h | 6 +++++ test/lit/passes/memory-packing-gc.wast | 33 ++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/ir/ReFinalize.cpp b/src/ir/ReFinalize.cpp index 9d68a9fe81a..50e5728c156 100644 --- a/src/ir/ReFinalize.cpp +++ b/src/ir/ReFinalize.cpp @@ -16,6 +16,7 @@ #include "ir/branch-utils.h" #include "ir/find_all.h" +#include "ir/type-updating.h" #include "ir/utils.h" namespace wasm { @@ -96,7 +97,19 @@ void ReFinalize::visitSwitch(Switch* curr) { void ReFinalize::visitCall(Call* curr) { curr->finalize(); } void ReFinalize::visitCallIndirect(CallIndirect* curr) { curr->finalize(); } void ReFinalize::visitLocalGet(LocalGet* curr) { curr->finalize(); } -void ReFinalize::visitLocalSet(LocalSet* curr) { curr->finalize(); } +void ReFinalize::visitLocalSet(LocalSet* curr) { + auto oldType = curr->type; + curr->finalize(); + if (curr->type == Type::unreachable && oldType != curr->type) { + // We changed the type of this set from reachable to unreachable. This can + // affect non-nullable local validation BLAH BLAH + auto localType = getFunction()->getLocalType(curr->index); + if (localType.isNonNullable()) { + // Perhaps just this local? But this is rare, so maybe not that bad. + needNonNullableLocalFixups = true; + } + } +} void ReFinalize::visitGlobalGet(GlobalGet* curr) { curr->finalize(); } void ReFinalize::visitGlobalSet(GlobalSet* curr) { curr->finalize(); } void ReFinalize::visitLoad(Load* curr) { curr->finalize(); } @@ -191,6 +204,12 @@ void ReFinalize::visitStringSliceIter(StringSliceIter* curr) { curr->finalize(); } +void ReFinalize::visitFunction(Function* curr) { + if (needNonNullableLocalFixups) { + TypeUpdating::handleNonDefaultableLocals(curr, *getModule()); + } +} + void ReFinalize::visitExport(Export* curr) { WASM_UNREACHABLE("unimp"); } void ReFinalize::visitGlobal(Global* curr) { WASM_UNREACHABLE("unimp"); } void ReFinalize::visitTable(Table* curr) { WASM_UNREACHABLE("unimp"); } diff --git a/src/ir/utils.h b/src/ir/utils.h index 72aa701bb77..4caf2757ee4 100644 --- a/src/ir/utils.h +++ b/src/ir/utils.h @@ -133,11 +133,17 @@ struct ReFinalize // bulk, tracking break value types so we just do a linear pass. std::unordered_map> breakTypes; + // In certain cases we may affect the validation of non-nullable locals. See + // ReFinalize::visitLocalSet(). + bool needNonNullableLocalFixups = false; + #define DELEGATE(CLASS_TO_VISIT) \ void visit##CLASS_TO_VISIT(CLASS_TO_VISIT* curr); #include "wasm-delegations.def" + void visitFunction(Function* curr); + void visitExport(Export* curr); void visitGlobal(Global* curr); void visitTable(Table* curr); diff --git a/test/lit/passes/memory-packing-gc.wast b/test/lit/passes/memory-packing-gc.wast index 547dec8076e..bafa158a83d 100644 --- a/test/lit/passes/memory-packing-gc.wast +++ b/test/lit/passes/memory-packing-gc.wast @@ -174,3 +174,36 @@ ) ) ) + +(module + (type $[i32] (array i32)) + + (memory $0 (shared 16 17)) + (data $0 (i32.const 37) "") + + (func $test + (local $temp (ref any)) + ;; The memory.init will trigger a ReFinalize. That will cause the if to + ;; become unreachable, after which the local.set is unreachable. We do not + ;; consider such sets to help non-nullable locals to validate (we do not emit + ;; unreachable code), so we need to fix up the non-nullable get afterwards). + (memory.init $0 + (i32.const 0) + (i32.const 8) + (i32.const 5) + ) + (local.set $temp + (array.new $[i32] + (if (result i32) + (i32.const 1) + (unreachable) + (unreachable) + ) + (i32.const 1) + ) + ) + (drop + (local.get $temp) + ) + ) +) From 2a7322219289f6f92b39afcbc75c401f0a901c14 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 18 Apr 2023 08:26:30 -0700 Subject: [PATCH 4/6] test --- test/lit/passes/memory-packing-gc.wast | 41 +++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/test/lit/passes/memory-packing-gc.wast b/test/lit/passes/memory-packing-gc.wast index bafa158a83d..00d2c75c3a9 100644 --- a/test/lit/passes/memory-packing-gc.wast +++ b/test/lit/passes/memory-packing-gc.wast @@ -178,15 +178,54 @@ (module (type $[i32] (array i32)) + ;; CHECK: (type $none_=>_none (func)) + + ;; CHECK: (memory $0 (shared 16 17)) (memory $0 (shared 16 17)) (data $0 (i32.const 37) "") + ;; CHECK: (func $test (type $none_=>_none) + ;; CHECK-NEXT: (local $temp anyref) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 8) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 5) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.tee $temp + ;; CHECK-NEXT: (block ;; (replaces something unreachable we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (ref.as_non_null + ;; CHECK-NEXT: (local.get $temp) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) (func $test (local $temp (ref any)) ;; The memory.init will trigger a ReFinalize. That will cause the if to ;; become unreachable, after which the local.set is unreachable. We do not ;; consider such sets to help non-nullable locals to validate (we do not emit - ;; unreachable code), so we need to fix up the non-nullable get afterwards). + ;; unreachable code), so we need to fix up the non-nullable get afterwards + ;; by making it nullable, and adding a ref.as_non_null). (memory.init $0 (i32.const 0) (i32.const 8) From b3640f23e51765413946e0c66b61e3500a4f0a95 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 18 Apr 2023 09:52:35 -0700 Subject: [PATCH 5/6] fix --- src/ir/ReFinalize.cpp | 1 + src/passes/LocalSubtyping.cpp | 53 ++++++++++++++++++----------------- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/ir/ReFinalize.cpp b/src/ir/ReFinalize.cpp index 50e5728c156..d78ae315534 100644 --- a/src/ir/ReFinalize.cpp +++ b/src/ir/ReFinalize.cpp @@ -106,6 +106,7 @@ void ReFinalize::visitLocalSet(LocalSet* curr) { auto localType = getFunction()->getLocalType(curr->index); if (localType.isNonNullable()) { // Perhaps just this local? But this is rare, so maybe not that bad. +std::cout << "nbeed\n"; needNonNullableLocalFixups = true; } } diff --git a/src/passes/LocalSubtyping.cpp b/src/passes/LocalSubtyping.cpp index 0d41434fce0..402157e5d2c 100644 --- a/src/passes/LocalSubtyping.cpp +++ b/src/passes/LocalSubtyping.cpp @@ -75,32 +75,6 @@ struct LocalSubtyping : public WalkerPass> { } } - // Find which vars can be non-nullable. - std::unordered_set cannotBeNonNullable; - - if (getModule()->features.hasGCNNLocals()) { - // If the feature is enabled then the only constraint is being able to - // read the default value - if it is readable, the local cannot become - // non-nullable. - for (auto& [get, sets] : localGraph.getSetses) { - auto index = get->index; - if (func->isVar(index) && - std::any_of(sets.begin(), sets.end(), [&](LocalSet* set) { - return set == nullptr; - })) { - cannotBeNonNullable.insert(index); - } - } - } else { - // Without GCNNLocals, validation rules follow the spec rules: all gets - // must be dominated structurally by sets, for the local to be non- - // nullable. - LocalStructuralDominance info(func, *getModule()); - for (auto index : info.nonDominatingIndices) { - cannotBeNonNullable.insert(index); - } - } - auto varBase = func->getVarIndexBase(); // Keep iterating while we find things to change. There can be chains like @@ -125,6 +99,33 @@ struct LocalSubtyping : public WalkerPass> { // the next step for knowing if there is more work to do. ReFinalize().walkFunctionInModule(func, getModule()); + // Find which vars can be non-nullable. Note that we must do this after + // refinalizing, as that operation can alter the types of locals. + std::unordered_set cannotBeNonNullable; + + if (getModule()->features.hasGCNNLocals()) { + // If the feature is enabled then the only constraint is being able to + // read the default value - if it is readable, the local cannot become + // non-nullable. + for (auto& [get, sets] : localGraph.getSetses) { + auto index = get->index; + if (func->isVar(index) && + std::any_of(sets.begin(), sets.end(), [&](LocalSet* set) { + return set == nullptr; + })) { + cannotBeNonNullable.insert(index); + } + } + } else { + // Without GCNNLocals, validation rules follow the spec rules: all gets + // must be dominated structurally by sets, for the local to be non- + // nullable. + LocalStructuralDominance info(func, *getModule()); + for (auto index : info.nonDominatingIndices) { + cannotBeNonNullable.insert(index); + } + } + // Second, find vars whose actual applied values allow a more specific // type. From cba429a0b637e74d55a265586682b5da353a2fd1 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 18 Apr 2023 09:52:48 -0700 Subject: [PATCH 6/6] fix --- src/ir/ReFinalize.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ir/ReFinalize.cpp b/src/ir/ReFinalize.cpp index d78ae315534..50e5728c156 100644 --- a/src/ir/ReFinalize.cpp +++ b/src/ir/ReFinalize.cpp @@ -106,7 +106,6 @@ void ReFinalize::visitLocalSet(LocalSet* curr) { auto localType = getFunction()->getLocalType(curr->index); if (localType.isNonNullable()) { // Perhaps just this local? But this is rare, so maybe not that bad. -std::cout << "nbeed\n"; needNonNullableLocalFixups = true; } }