From 104ed2756f90ec2263f6b11a4835cbd96f19c360 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 12 Jun 2020 17:45:20 -0700 Subject: [PATCH 1/8] wip [ci skip] --- src/passes/Asyncify.cpp | 40 +++- ...@foo_pass-arg=asyncify-ignore-indirect.txt | 171 ++++++++++++++++++ ...foo_pass-arg=asyncify-ignore-indirect.wast | 18 ++ 3 files changed, 220 insertions(+), 9 deletions(-) create mode 100644 test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.txt create mode 100644 test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.wast diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index 8dcbb01bb67..017afb7c926 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -224,6 +224,15 @@ // will never need to be unwound with an indirect call somewhere in it. // If that is true for your codebase, then this can be extremely useful // as otherwise it looks like any indirect call can go to a lot of places. +// Note that this does not apply to indirect calls from functions in the +// only-list or add-list, see below: even if you ignore indirect calls in +// general, adding a function explicitly to be instrumented implies that +// you want full support there. This is necessary if you are manually +// adding certain functions then for an indirect call to work we need +// support on both sides - in the function itself, and in places that call +// it (where we don't know which function is being called). In other words, +// you can add functions to the list both to add support for being called, +// and for calling. // // --pass-arg=asyncify-asserts // @@ -478,6 +487,7 @@ class ModuleAnalyzer { // that call it are instrumented. This is not done for the bottom. bool isTopMostRuntime = false; bool inRemoveList = false; + bool addedFromList = false; }; typedef std::map Map; @@ -634,7 +644,12 @@ class ModuleAnalyzer { // Only the functions in the only-list can change the state. for (auto& func : module.functions) { if (!func->imported()) { - map[func.get()].canChangeState = onlyList.match(func->name); + auto& info = map[func.get()]; + bool matched = onlyList.match(func->name); + info.canChangeState = matched; + if (matched) { + info.addedFromList = true; + } } } } @@ -642,7 +657,9 @@ class ModuleAnalyzer { if (!addListInput.empty()) { for (auto& func : module.functions) { if (!func->imported() && addList.match(func->name)) { - map[func.get()].canChangeState = true; + auto& info = map[func.get()]; + info.canChangeState = true; + info.addedFromList = true; } } } @@ -657,7 +674,7 @@ class ModuleAnalyzer { return info.canChangeState && !info.isTopMostRuntime; } - bool canChangeState(Expression* curr) { + bool canChangeState(Expression* curr, Function* func) { // Look inside to see if we call any of the things we know can change the // state. // TODO: caching, this is O(N^2) @@ -684,15 +701,12 @@ class ModuleAnalyzer { } } void visitCallIndirect(CallIndirect* curr) { - if (canIndirectChangeState) { - canChangeState = true; - } - // TODO optimize the other case, at least by type + hasIndirectCall = true; } Module* module; ModuleAnalyzer* analyzer; Map* map; - bool canIndirectChangeState; + bool hasIndirectCall = false; bool canChangeState = false; bool isBottomMostRuntime = false; }; @@ -700,11 +714,19 @@ class ModuleAnalyzer { walker.module = &module; walker.analyzer = this; walker.map = ↦ - walker.canIndirectChangeState = canIndirectChangeState; walker.walk(curr); if (walker.isBottomMostRuntime) { walker.canChangeState = false; } + // An indirect call is normally ignored if we are ignoring indirect calls. + // However, if the function we are inside was specifically added by the user + // (in the only-list or the add-list) then we assume it may change the + // state. This allows adding functions to actually let some indirect calls + // work (as the support needs to be on both sides, the caller and the + // callee). + if (walker.hasIndirectCall && (canIndirectChangeState || map[func].addedFromList)) { + walker.canChangeState = false; + } return walker.canChangeState; } diff --git a/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.txt b/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.txt new file mode 100644 index 00000000000..121d0a6ee7d --- /dev/null +++ b/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.txt @@ -0,0 +1,171 @@ +(module + (type $none_=>_none (func)) + (type $i32_=>_none (func (param i32))) + (type $none_=>_i32 (func (result i32))) + (import "env" "import" (func $import)) + (memory $0 1 2) + (table $0 1 funcref) + (global $__asyncify_state (mut i32) (i32.const 0)) + (global $__asyncify_data (mut i32) (i32.const 0)) + (export "asyncify_start_unwind" (func $asyncify_start_unwind)) + (export "asyncify_stop_unwind" (func $asyncify_stop_unwind)) + (export "asyncify_start_rewind" (func $asyncify_start_rewind)) + (export "asyncify_stop_rewind" (func $asyncify_stop_rewind)) + (export "asyncify_get_state" (func $asyncify_get_state)) + (func $foo + (local $0 i32) + (local $1 i32) + (if + (i32.eq + (global.get $__asyncify_state) + (i32.const 2) + ) + (nop) + ) + (local.tee $0 + (block $__asyncify_unwind + (block + (block + (if + (i32.eq + (global.get $__asyncify_state) + (i32.const 2) + ) + (block + (i32.store + (global.get $__asyncify_data) + (i32.add + (i32.load + (global.get $__asyncify_data) + ) + (i32.const -4) + ) + ) + (local.set $1 + (i32.load + (i32.load + (global.get $__asyncify_data) + ) + ) + ) + ) + ) + (if + (i32.eq + (global.get $__asyncify_state) + (i32.const 0) + ) + (block + (call $nothing) + (call_indirect (type $none_=>_none) + (i32.const 0) + ) + ) + ) + ) + (return) + ) + ) + ) + (block + (i32.store + (i32.load + (global.get $__asyncify_data) + ) + (local.get $0) + ) + (i32.store + (global.get $__asyncify_data) + (i32.add + (i32.load + (global.get $__asyncify_data) + ) + (i32.const 4) + ) + ) + ) + (nop) + ) + (func $bar + (call $nothing) + (call_indirect (type $none_=>_none) + (i32.const 0) + ) + ) + (func $nothing + (nop) + ) + (func $asyncify_start_unwind (param $0 i32) + (global.set $__asyncify_state + (i32.const 1) + ) + (global.set $__asyncify_data + (local.get $0) + ) + (if + (i32.gt_u + (i32.load + (global.get $__asyncify_data) + ) + (i32.load offset=4 + (global.get $__asyncify_data) + ) + ) + (unreachable) + ) + ) + (func $asyncify_stop_unwind + (global.set $__asyncify_state + (i32.const 0) + ) + (if + (i32.gt_u + (i32.load + (global.get $__asyncify_data) + ) + (i32.load offset=4 + (global.get $__asyncify_data) + ) + ) + (unreachable) + ) + ) + (func $asyncify_start_rewind (param $0 i32) + (global.set $__asyncify_state + (i32.const 2) + ) + (global.set $__asyncify_data + (local.get $0) + ) + (if + (i32.gt_u + (i32.load + (global.get $__asyncify_data) + ) + (i32.load offset=4 + (global.get $__asyncify_data) + ) + ) + (unreachable) + ) + ) + (func $asyncify_stop_rewind + (global.set $__asyncify_state + (i32.const 0) + ) + (if + (i32.gt_u + (i32.load + (global.get $__asyncify_data) + ) + (i32.load offset=4 + (global.get $__asyncify_data) + ) + ) + (unreachable) + ) + ) + (func $asyncify_get_state (result i32) + (global.get $__asyncify_state) + ) +) diff --git a/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.wast b/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.wast new file mode 100644 index 00000000000..a7541d5adc3 --- /dev/null +++ b/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.wast @@ -0,0 +1,18 @@ +(module + (type $t (func)) + (memory 1 2) + (table 1 funcref) + (elem (i32.const 0)) + (import "env" "import" (func $import)) + (func $foo ;; doesn't look like it needs instrumentation, but in add list + (call $nothing) + (call_indirect (type $t) (i32.const 0)) + ) + (func $bar ;; doesn't look like it needs instrumentation, and not in add list + (call $nothing) + (call_indirect (type $t) (i32.const 0)) + ) + (func $nothing + ) +) + From 9f30d5412ad8ac5f1ebd8e5feaa3ee06426a1db3 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 12 Jun 2020 18:52:59 -0700 Subject: [PATCH 2/8] finish --- src/passes/Asyncify.cpp | 10 ++--- ...@foo_pass-arg=asyncify-ignore-indirect.txt | 44 ++++++++++++++----- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index 017afb7c926..fe2f475a7e4 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -725,7 +725,7 @@ class ModuleAnalyzer { // work (as the support needs to be on both sides, the caller and the // callee). if (walker.hasIndirectCall && (canIndirectChangeState || map[func].addedFromList)) { - walker.canChangeState = false; + walker.canChangeState = true; } return walker.canChangeState; } @@ -836,7 +836,7 @@ struct AsyncifyFlow : public Pass { Index callIndex = 0; Expression* process(Expression* curr) { - if (!analyzer->canChangeState(curr)) { + if (!analyzer->canChangeState(curr, func)) { return makeMaybeSkip(curr); } // The IR is in flat form, which makes this much simpler: there are no @@ -878,12 +878,12 @@ struct AsyncifyFlow : public Pass { Index i = 0; auto& list = block->list; while (i < list.size()) { - if (analyzer->canChangeState(list[i])) { + if (analyzer->canChangeState(list[i], func)) { list[i] = process(list[i]); i++; } else { Index end = i + 1; - while (end < list.size() && !analyzer->canChangeState(list[end])) { + while (end < list.size() && !analyzer->canChangeState(list[end], func)) { end++; } // We have a range of [i, end) in which the state cannot change, @@ -908,7 +908,7 @@ struct AsyncifyFlow : public Pass { } else if (auto* iff = curr->dynCast()) { // The state change cannot be in the condition due to flat form, so it // must be in one of the children. - assert(!analyzer->canChangeState(iff->condition)); + assert(!analyzer->canChangeState(iff->condition, func)); // We must linearize this, which means we pass through both arms if we // are rewinding. if (!iff->ifFalse) { diff --git a/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.txt b/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.txt index 121d0a6ee7d..dc7c3e1961d 100644 --- a/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.txt +++ b/test/passes/asyncify_pass-arg=asyncify-addlist@foo_pass-arg=asyncify-ignore-indirect.txt @@ -22,8 +22,8 @@ ) (nop) ) - (local.tee $0 - (block $__asyncify_unwind + (local.set $0 + (block $__asyncify_unwind (result i32) (block (block (if @@ -50,16 +50,40 @@ ) ) ) - (if - (i32.eq - (global.get $__asyncify_state) - (i32.const 0) - ) - (block - (call $nothing) - (call_indirect (type $none_=>_none) + (block + (if + (i32.eq + (global.get $__asyncify_state) (i32.const 0) ) + (call $nothing) + ) + (if + (if (result i32) + (i32.eq + (global.get $__asyncify_state) + (i32.const 0) + ) + (i32.const 1) + (i32.eq + (local.get $1) + (i32.const 0) + ) + ) + (block + (call_indirect (type $none_=>_none) + (i32.const 0) + ) + (if + (i32.eq + (global.get $__asyncify_state) + (i32.const 1) + ) + (br $__asyncify_unwind + (i32.const 0) + ) + ) + ) ) ) ) From 831fa36afe51cf128332bdc1acb42ed9f1a694de Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 12 Jun 2020 18:53:10 -0700 Subject: [PATCH 3/8] format --- src/passes/Asyncify.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index fe2f475a7e4..b9d1dd83b83 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -700,9 +700,7 @@ class ModuleAnalyzer { canChangeState = true; } } - void visitCallIndirect(CallIndirect* curr) { - hasIndirectCall = true; - } + void visitCallIndirect(CallIndirect* curr) { hasIndirectCall = true; } Module* module; ModuleAnalyzer* analyzer; Map* map; @@ -724,7 +722,8 @@ class ModuleAnalyzer { // state. This allows adding functions to actually let some indirect calls // work (as the support needs to be on both sides, the caller and the // callee). - if (walker.hasIndirectCall && (canIndirectChangeState || map[func].addedFromList)) { + if (walker.hasIndirectCall && + (canIndirectChangeState || map[func].addedFromList)) { walker.canChangeState = true; } return walker.canChangeState; @@ -883,7 +882,8 @@ struct AsyncifyFlow : public Pass { i++; } else { Index end = i + 1; - while (end < list.size() && !analyzer->canChangeState(list[end], func)) { + while (end < list.size() && + !analyzer->canChangeState(list[end], func)) { end++; } // We have a range of [i, end) in which the state cannot change, From 46fa5033d304ad11299c2ef201704c7051b61e24 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Sat, 13 Jun 2020 15:01:06 -0700 Subject: [PATCH 4/8] comment --- src/passes/Asyncify.cpp | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index b9d1dd83b83..ffce95e8289 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -224,15 +224,6 @@ // will never need to be unwound with an indirect call somewhere in it. // If that is true for your codebase, then this can be extremely useful // as otherwise it looks like any indirect call can go to a lot of places. -// Note that this does not apply to indirect calls from functions in the -// only-list or add-list, see below: even if you ignore indirect calls in -// general, adding a function explicitly to be instrumented implies that -// you want full support there. This is necessary if you are manually -// adding certain functions then for an indirect call to work we need -// support on both sides - in the function itself, and in places that call -// it (where we don't know which function is being called). In other words, -// you can add functions to the list both to add support for being called, -// and for calling. // // --pass-arg=asyncify-asserts // @@ -274,6 +265,27 @@ // If the "only-list" is provided, then *only* the functions in the list // will be instrumented, and nothing else. // +// Note that there are two types of instrumentation that happen for each +// function: if foo() can be part of a pause/resume operation, then we need to +// instrument code inside it to support pausing and resuming, but also, we need +// callers of the function to instrument calls to it. Normally this is already +// taken care of, as the callers need to instrumented as well anyhow. However, +// the ignore-indirect option makes things more complicated, since we can't tell +// where all the calls to foo() are - all we see a indirect calls, that do not +// refer to foo() by name. To make it possible for you to use the add-list or +// only-list with ignore-indirect, those lists affect *both* kinds of +// instrumentation. That is, if parent() calls foo() indirectly, and you add +// parent() to the add-list, then the indirect calls in parent() will be +// instrumented to support pausing/resuming, even if ignore-indirect is set. +// Typically you don't need to think about this, and just add all the functions +// that can be on the stack while pausing - what this means is that when you do +// so, indirect calls will just work. (The cost is that an instrumented function +// will check for pausing at all indirect calls, which may be unnecessary in +// some cases; but this is an instrumented function anyhow, and indirect calls +// are slower anyhow, so this simple model seems good enough - a more complex +// model where you can specify "instrument, but not indirect calls from me" +// would likely have little benefit.) +// // TODO When wasm has GC, extending the live ranges of locals can keep things // alive unnecessarily. We may want to set locals to null at the end // of their original range. From aa3a58546628708ccaceb87e60110efbcae54b99 Mon Sep 17 00:00:00 2001 From: "Alon Zakai (kripken)" Date: Mon, 15 Jun 2020 16:09:51 -0700 Subject: [PATCH 5/8] comment --- src/passes/Asyncify.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index ffce95e8289..a58efcc7aff 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -269,7 +269,7 @@ // function: if foo() can be part of a pause/resume operation, then we need to // instrument code inside it to support pausing and resuming, but also, we need // callers of the function to instrument calls to it. Normally this is already -// taken care of, as the callers need to instrumented as well anyhow. However, +// taken care of as the callers need to be instrumented as well anyhow. However, // the ignore-indirect option makes things more complicated, since we can't tell // where all the calls to foo() are - all we see a indirect calls, that do not // refer to foo() by name. To make it possible for you to use the add-list or From 78c821cdf7c8e0c5d3ea717c62b90b7839bed4f6 Mon Sep 17 00:00:00 2001 From: "Alon Zakai (kripken)" Date: Mon, 15 Jun 2020 16:13:10 -0700 Subject: [PATCH 6/8] comment --- src/passes/Asyncify.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index a58efcc7aff..e4192712333 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -729,11 +729,10 @@ class ModuleAnalyzer { walker.canChangeState = false; } // An indirect call is normally ignored if we are ignoring indirect calls. - // However, if the function we are inside was specifically added by the user - // (in the only-list or the add-list) then we assume it may change the - // state. This allows adding functions to actually let some indirect calls - // work (as the support needs to be on both sides, the caller and the - // callee). + // However, see the docs at the top: if the function we are inside was + // specifically added by the user (in the only-list or the add-list) then we + // instrument indirect calls from it (this allows specifically allowing some + // indirect calls but not others). if (walker.hasIndirectCall && (canIndirectChangeState || map[func].addedFromList)) { walker.canChangeState = true; From 7dce99279ea295f071252094245c4cf7ffd30cf8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 16 Jun 2020 16:07:42 -0700 Subject: [PATCH 7/8] Update src/passes/Asyncify.cpp Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com> --- src/passes/Asyncify.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index e4192712333..412fbac6ab0 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -271,7 +271,7 @@ // callers of the function to instrument calls to it. Normally this is already // taken care of as the callers need to be instrumented as well anyhow. However, // the ignore-indirect option makes things more complicated, since we can't tell -// where all the calls to foo() are - all we see a indirect calls, that do not +// where all the calls to foo() are - all we see are indirect calls that do not // refer to foo() by name. To make it possible for you to use the add-list or // only-list with ignore-indirect, those lists affect *both* kinds of // instrumentation. That is, if parent() calls foo() indirectly, and you add From 088088edf77fb9d59f508d9eb04e4dda36b16374 Mon Sep 17 00:00:00 2001 From: "Alon Zakai (kripken)" Date: Wed, 17 Jun 2020 07:13:57 -0700 Subject: [PATCH 8/8] fix and merge condition --- src/passes/Asyncify.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/passes/Asyncify.cpp b/src/passes/Asyncify.cpp index 412fbac6ab0..2708ae4e0ee 100644 --- a/src/passes/Asyncify.cpp +++ b/src/passes/Asyncify.cpp @@ -725,9 +725,6 @@ class ModuleAnalyzer { walker.analyzer = this; walker.map = ↦ walker.walk(curr); - if (walker.isBottomMostRuntime) { - walker.canChangeState = false; - } // An indirect call is normally ignored if we are ignoring indirect calls. // However, see the docs at the top: if the function we are inside was // specifically added by the user (in the only-list or the add-list) then we @@ -737,7 +734,8 @@ class ModuleAnalyzer { (canIndirectChangeState || map[func].addedFromList)) { walker.canChangeState = true; } - return walker.canChangeState; + // The bottom-most runtime can never change the state. + return walker.canChangeState && !walker.isBottomMostRuntime; } FakeGlobalHelper fakeGlobals;