diff --git a/src/passes/InstrumentBranchHints.cpp b/src/passes/InstrumentBranchHints.cpp index 656397a5ad1..0fa6f3009b6 100644 --- a/src/passes/InstrumentBranchHints.cpp +++ b/src/passes/InstrumentBranchHints.cpp @@ -96,6 +96,7 @@ // #include "ir/drop.h" +#include "ir/effects.h" #include "ir/eh-utils.h" #include "ir/find_all.h" #include "ir/local-graph.h" @@ -272,6 +273,8 @@ struct InstrumentationProcessor : public WalkerPass> { // The condition before the instrumentation (a pointer to it, so we can // replace it). Expression** originalCondition; + // The local that the original condition is stored in temporarily. + Index tempLocal; // The call to the logging that the instrumentation added. Call* call; }; @@ -330,7 +333,7 @@ struct InstrumentationProcessor : public WalkerPass> { return {}; } // Great, this is indeed a prior instrumentation. - return Instrumentation{&set->value, call}; + return Instrumentation{&set->value, set->index, call}; } }; @@ -374,7 +377,27 @@ struct DeInstrumentBranchHints // IR, and the original condition is still used in another place, until // we remove the logging calls; since we will remove the calls anyhow, we // just need some valid IR there). - std::swap(curr->condition, *info->originalCondition); + // + // Check for dangerous effects in the condition we are about to replace, + // to avoid a situation where the condition looks like this: + // + // (set $temp (original condition)) + // ..effects.. + // (local.get $temp) + // + // We cannot replace all this with the original condition, as it would + // remove the effects. (Even in that case we will remove the actual call + // to log the branch hint, below, so this just prevents some cleanup that + // is normally safe - the cleanup is mainly useful to allow inspection of + // testcases for debugging.) + EffectAnalyzer effects(getPassOptions(), *getModule(), curr->condition); + // The only condition we allow is a write to the temp local from the + // instrumentation, which getInstrumentation() verified has no other uses + // than us. + effects.localsWritten.erase(info->tempLocal); + if (!effects.hasUnremovableSideEffects()) { + std::swap(curr->condition, *info->originalCondition); + } } } @@ -403,6 +426,21 @@ struct DeInstrumentBranchHints } } } + + void doWalkModule(Module* module) { + auto logBranchImport = getLogBranchImport(module); + if (!logBranchImport) { + Fatal() + << "No branch hint logging import found. Was this code instrumented?"; + } + + // Mark the log-branch import as having no side effects - we are removing it + // entirely here, and its effect should not stop us when we compute effects. + module->getFunction(logBranchImport)->effects = + std::make_shared(getPassOptions(), *module); + + InstrumentationProcessor::doWalkModule(module); + } }; } // anonymous namespace diff --git a/test/lit/passes/deinstrument-branch-hints.wast b/test/lit/passes/deinstrument-branch-hints.wast index 6d619c4b28b..3f9019028e7 100644 --- a/test/lit/passes/deinstrument-branch-hints.wast +++ b/test/lit/passes/deinstrument-branch-hints.wast @@ -7,6 +7,8 @@ ;; CHECK: (type $1 (func (param i32 i32 i32))) + ;; CHECK: (type $2 (func (result i32))) + ;; CHECK: (import "fuzzing-support" "log-branch" (func $log (type $1) (param i32 i32 i32))) (import "fuzzing-support" "log-branch" (func $log (param i32 i32 i32))) @@ -164,4 +166,57 @@ ) ) ) + + ;; CHECK: (func $if-unreachable (type $2) (result i32) + ;; CHECK-NEXT: (local $0 i32) + ;; CHECK-NEXT: (block $block (result i32) + ;; CHECK-NEXT: (br_if $block + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (local.set $0 + ;; CHECK-NEXT: (i32.const 42) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (local.get $0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $if-unreachable (result i32) + (local $0 i32) + ;; The unreachable here must be executed. Normally we replace the br_if's + ;; entire condition, but here we only remove the call to $log. + (block $block (result i32) + (br_if $block + (i32.const 0) + (block (result i32) + (block + (local.set $0 + (i32.const 42) + ) + (if + (i32.const 1) + (then + (unreachable) + ) + ) + ) + (call $log + (i32.const 0) + (i32.const 0) + (local.get $0) + ) + (local.get $0) + ) + ) + ) + ) )