From 39548d29f3ee45f9d768ec39ab3bfc6e2c393577 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 22 Mar 2024 14:36:24 -0700 Subject: [PATCH 01/14] work --- src/ir/localize.h | 19 +++++---- src/tools/fuzzing/fuzzing.cpp | 72 +++++++++++++++++++++++++++-------- 2 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/ir/localize.h b/src/ir/localize.h index d44fb9be5f3..4dbd43a736e 100644 --- a/src/ir/localize.h +++ b/src/ir/localize.h @@ -110,7 +110,7 @@ struct ChildLocalizer { // Move the child out, and put an unreachable in its place (note that we // don't need an actual set here, as there is no value to set to a // local). - sets.push_back(child); + movedChildren.push_back(child); *childp = builder.makeUnreachable(); hasUnreachableChild = true; continue; @@ -121,7 +121,7 @@ struct ChildLocalizer { // (The only reason we still need them is that they may be needed for // validation, e.g. if one contains a break to a block that is the only // reason the block has type none.) - sets.push_back(builder.makeDrop(child)); + movedChildren.push_back(builder.makeDrop(child)); *childp = builder.makeUnreachable(); continue; } @@ -139,23 +139,23 @@ struct ChildLocalizer { } if (needLocal) { auto local = builder.addVar(func, child->type); - sets.push_back(builder.makeLocalSet(local, child)); + movedChildren.push_back(builder.makeLocalSet(local, child)); *childp = builder.makeLocalGet(local, child->type); } } } // Helper that gets a replacement for the parent: a block containing the - // sets + the parent. This will not contain the parent if we don't need it - // (if it was never reached). + // movedChildren + the parent. This will not contain the parent if we don't + // need it (if it was never reached). Expression* getReplacement() { - if (sets.empty()) { + if (movedChildren.empty()) { // Nothing to add. return parent; } auto* block = Builder(wasm).makeBlock(); - block->list.set(sets); + block->list.set(movedChildren); if (hasUnreachableChild) { // If there is an unreachable child then we do not need the parent at all, // and we know the type is unreachable. @@ -168,11 +168,14 @@ struct ChildLocalizer { return block; } + // The vector of moved children may be useful to some users directly, e.g. if + // all they care about are the children and not the parent. + std::vector movedChildren; + private: Expression* parent; Module& wasm; - std::vector sets; bool hasUnreachableChild = false; }; diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 1c4ee4cc5f4..2acb67b4ec9 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -16,6 +16,8 @@ #include "tools/fuzzing.h" #include "ir/gc-type-utils.h" +#include "ir/iteration.h" +#include "ir/localize.h" #include "ir/local-structural-dominance.h" #include "ir/module-utils.h" #include "ir/subtypes.h" @@ -929,7 +931,6 @@ void TranslateToFuzzReader::mutate(Function* func) { percentChance = std::max(percentChance, Index(3)); struct Modder : public PostWalker> { - Module& wasm; TranslateToFuzzReader& parent; Index percentChance; @@ -937,8 +938,8 @@ void TranslateToFuzzReader::mutate(Function* func) { // executed, so we don't want to do it all the time even in a big function. bool allowUnreachable; - Modder(Module& wasm, TranslateToFuzzReader& parent, Index percentChance) - : wasm(wasm), parent(parent), percentChance(percentChance) { + Modder(TranslateToFuzzReader& parent, Index percentChance) + : parent(parent), percentChance(percentChance) { // If the parent allows it then sometimes replace with an unreachable, and // sometimes not. Even if we allow it, only do it in certain functions // (half the time) and only do it rarely (see below). @@ -948,29 +949,70 @@ void TranslateToFuzzReader::mutate(Function* func) { void visitExpression(Expression* curr) { if (parent.upTo(100) < percentChance && parent.canBeArbitrarilyReplaced(curr)) { - if (allowUnreachable && parent.oneIn(20)) { + // We can replace in various modes, see below. Generate a random number + // up to 100 to help us there. + int mode = parent.upTo(100); + + if (allowUnreachable && mode < 5) { replaceCurrent(parent.make(Type::unreachable)); return; } + // For constants, perform only a small tweaking in some cases. + // TODO: more minor tweaks to immediates, like making a load atomic or + // not, changing an offset, etc. if (auto* c = curr->dynCast()) { - if (parent.oneIn(2)) { + if (mode < 50) { c->value = parent.tweak(c->value); - return; + } else { + // Just replace the entire thing. + replaceCurrent(parent.make(curr->type)); } + return; } - // TODO: more minor tweaks to immediates, like making a load atomic or - // not, changing an offset, etc. - // Perform a general replacement. (This is not always valid due to - // nesting of labels, but we'll fix that up later.) Note that make() - // picks a subtype, so this has a chance to replace us with anything - // that is valid to put here. - replaceCurrent(parent.make(curr->type)); + + // Generate a replacement for the expression, and by default replace all + // of |curr|, including children, with that replacement, but in some + // cases we can do more subtle things. + // + // Note that such a replacement is not always valid due to nesting of + // labels, but we'll fix that up later. Note also that make() picks a + // subtype, so this has a chance to replace us with anything that is + // valid to put here. + auto* rep = parent.make(curr->type); + if (mode < 33 && rep->type != Type::none) { + // This has a non-none type. Replace the output, keeping the + // expression and its children before in a drop. This "interposes" + // between this expression and its parent. + // TODO: Ideally the new expression here could consume |curr| as a + // child. + rep = parent.builder.makeSequence(parent.builder.makeDrop(curr), + rep); + } else if (mode > 66 && + !Properties::isControlFlowStructure(curr) && + ChildIterator(curr).getNumChildren() > 0) { + // This is a normal (non-control-flow) expression with at least one + // child. "Interpose" between the children and this expression by + // keeping them and appending something else. + // TODO: Ideally the new expression here could consume the children. + + // Generate sets of the children to locals, and keep those. + auto sets = ChildLocalizer(curr, + getFunction(), + *getModule(), + PassOptions::getWithoutOptimization()).movedChildren; + auto* block = parent.builder.makeBlock(sets); + block->list.push_back(rep); + block->finalize(); + rep = block; + } + replaceCurrent(rep); } } }; - Modder modder(wasm, *this, percentChance); - modder.walk(func->body); + + Modder modder(*this, percentChance); + modder.walkFunctionInModule(func, &wasm); } void TranslateToFuzzReader::fixAfterChanges(Function* func) { From cc20f036ad791f9fadc3d28103258f09c7299662 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 22 Mar 2024 14:36:36 -0700 Subject: [PATCH 02/14] test --- test/passes/fuzz_metrics_noprint.bin.txt | 54 +++++++------- ...e-to-fuzz_all-features_metrics_noprint.txt | 74 +++++++++++-------- 2 files changed, 71 insertions(+), 57 deletions(-) diff --git a/test/passes/fuzz_metrics_noprint.bin.txt b/test/passes/fuzz_metrics_noprint.bin.txt index 703c2aeb411..e20068d52e6 100644 --- a/test/passes/fuzz_metrics_noprint.bin.txt +++ b/test/passes/fuzz_metrics_noprint.bin.txt @@ -1,34 +1,34 @@ total - [exports] : 29 - [funcs] : 39 + [exports] : 18 + [funcs] : 21 [globals] : 9 [imports] : 4 [memories] : 1 [memory-data] : 2 - [table-data] : 6 + [table-data] : 7 [tables] : 1 [tags] : 0 - [total] : 5494 - [vars] : 119 - Binary : 400 - Block : 892 - Break : 210 - Call : 232 - CallIndirect : 12 - Const : 898 - Drop : 49 - GlobalGet : 421 - GlobalSet : 333 - If : 289 - Load : 113 - LocalGet : 434 - LocalSet : 306 - Loop : 118 - Nop : 85 - RefFunc : 6 - Return : 62 - Select : 52 - Store : 45 - Switch : 1 - Unary : 380 - Unreachable : 156 + [total] : 12378 + [vars] : 79 + Binary : 892 + Block : 1963 + Break : 478 + Call : 288 + CallIndirect : 131 + Const : 2052 + Drop : 89 + GlobalGet : 899 + GlobalSet : 714 + If : 657 + Load : 246 + LocalGet : 1115 + LocalSet : 841 + Loop : 317 + Nop : 155 + RefFunc : 7 + Return : 107 + Select : 85 + Store : 113 + Switch : 3 + Unary : 883 + Unreachable : 343 diff --git a/test/passes/translate-to-fuzz_all-features_metrics_noprint.txt b/test/passes/translate-to-fuzz_all-features_metrics_noprint.txt index 9697da8bd15..241d8471879 100644 --- a/test/passes/translate-to-fuzz_all-features_metrics_noprint.txt +++ b/test/passes/translate-to-fuzz_all-features_metrics_noprint.txt @@ -1,6 +1,6 @@ total [exports] : 3 - [funcs] : 6 + [funcs] : 5 [globals] : 1 [imports] : 5 [memories] : 1 @@ -8,35 +8,49 @@ total [table-data] : 1 [tables] : 1 [tags] : 2 - [total] : 314 - [vars] : 38 - ArrayNew : 2 - ArrayNewFixed : 1 + [total] : 661 + [vars] : 21 + ArrayGet : 1 + ArrayLen : 1 + ArrayNew : 4 + ArrayNewFixed : 6 AtomicFence : 1 - Binary : 58 - Block : 28 - Break : 6 - Call : 10 - Const : 72 - Drop : 3 - GlobalGet : 10 - GlobalSet : 10 + AtomicRMW : 1 + Binary : 87 + Block : 78 + Break : 17 + Call : 11 + Const : 125 + DataDrop : 1 + Drop : 7 + GlobalGet : 26 + GlobalSet : 26 I31Get : 1 - If : 7 - Load : 18 - LocalGet : 36 - LocalSet : 21 - Loop : 1 - Nop : 2 - RefEq : 1 - RefFunc : 2 - RefI31 : 2 - RefNull : 1 - Return : 2 - Select : 1 - Store : 1 - StructGet : 1 + If : 24 + Load : 22 + LocalGet : 65 + LocalSet : 38 + Loop : 9 + MemoryCopy : 1 + Nop : 9 + RefAs : 8 + RefCast : 1 + RefEq : 2 + RefFunc : 1 + RefI31 : 3 + RefIsNull : 2 + RefNull : 13 + RefTest : 1 + Return : 4 + SIMDExtract : 3 + SIMDLoad : 1 + Select : 2 + Store : 4 + StructGet : 2 StructNew : 3 - TupleMake : 2 - Unary : 6 - Unreachable : 5 + Throw : 1 + Try : 1 + TupleExtract : 2 + TupleMake : 5 + Unary : 28 + Unreachable : 13 From 8c5d83e5afe03e2ed70df614c3f23869239633c3 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 22 Mar 2024 14:50:46 -0700 Subject: [PATCH 03/14] notes --- src/tools/fuzzing/fuzzing.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 2acb67b4ec9..2b3a36919bc 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -972,7 +972,7 @@ void TranslateToFuzzReader::mutate(Function* func) { } // Generate a replacement for the expression, and by default replace all - // of |curr|, including children, with that replacement, but in some + // of |curr| (including children) with that replacement, but in some // cases we can do more subtle things. // // Note that such a replacement is not always valid due to nesting of @@ -982,26 +982,28 @@ void TranslateToFuzzReader::mutate(Function* func) { auto* rep = parent.make(curr->type); if (mode < 33 && rep->type != Type::none) { // This has a non-none type. Replace the output, keeping the - // expression and its children before in a drop. This "interposes" - // between this expression and its parent. + // expression and its children in a drop. This "interposes" between + // this expression and its parent. // TODO: Ideally the new expression here could consume |curr| as a - // child. + // child, though if so we should still keep a decent chance to + // just drop this expression (as a drop enables passes to think + // they can remove more code, by marking the output "unused"). rep = parent.builder.makeSequence(parent.builder.makeDrop(curr), rep); - } else if (mode > 66 && + } else if (mode >= 66 && !Properties::isControlFlowStructure(curr) && ChildIterator(curr).getNumChildren() > 0) { // This is a normal (non-control-flow) expression with at least one // child. "Interpose" between the children and this expression by - // keeping them and appending something else. - // TODO: Ideally the new expression here could consume the children. - - // Generate sets of the children to locals, and keep those. + // keeping them and replacing the parent |curr|. We do this by + // generating sets of the children to locals, and keeping those. auto sets = ChildLocalizer(curr, getFunction(), *getModule(), PassOptions::getWithoutOptimization()).movedChildren; auto* block = parent.builder.makeBlock(sets); + // TODO: Ideally the new expression |rep| could consume the children: + // all we need is for it to do local.get of the temp locals. block->list.push_back(rep); block->finalize(); rep = block; From ac0928c8a78d0245f2c7fb461a524e1d9da29bd9 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 22 Mar 2024 14:51:03 -0700 Subject: [PATCH 04/14] format --- src/tools/fuzzing/fuzzing.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 2b3a36919bc..37a6049b75d 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -17,8 +17,8 @@ #include "tools/fuzzing.h" #include "ir/gc-type-utils.h" #include "ir/iteration.h" -#include "ir/localize.h" #include "ir/local-structural-dominance.h" +#include "ir/localize.h" #include "ir/module-utils.h" #include "ir/subtypes.h" #include "ir/type-updating.h" @@ -988,19 +988,18 @@ void TranslateToFuzzReader::mutate(Function* func) { // child, though if so we should still keep a decent chance to // just drop this expression (as a drop enables passes to think // they can remove more code, by marking the output "unused"). - rep = parent.builder.makeSequence(parent.builder.makeDrop(curr), - rep); - } else if (mode >= 66 && - !Properties::isControlFlowStructure(curr) && + rep = parent.builder.makeSequence(parent.builder.makeDrop(curr), rep); + } else if (mode >= 66 && !Properties::isControlFlowStructure(curr) && ChildIterator(curr).getNumChildren() > 0) { // This is a normal (non-control-flow) expression with at least one // child. "Interpose" between the children and this expression by // keeping them and replacing the parent |curr|. We do this by // generating sets of the children to locals, and keeping those. auto sets = ChildLocalizer(curr, - getFunction(), - *getModule(), - PassOptions::getWithoutOptimization()).movedChildren; + getFunction(), + *getModule(), + PassOptions::getWithoutOptimization()) + .movedChildren; auto* block = parent.builder.makeBlock(sets); // TODO: Ideally the new expression |rep| could consume the children: // all we need is for it to do local.get of the temp locals. From 7ba190d42aec8458ba95e21f60295aca9f8c466b Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 22 Mar 2024 14:59:06 -0700 Subject: [PATCH 05/14] fix --- src/ir/localize.h | 19 ++++++++----------- src/tools/fuzzing/fuzzing.cpp | 34 ++++++++++++++++------------------ 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/src/ir/localize.h b/src/ir/localize.h index 4dbd43a736e..d44fb9be5f3 100644 --- a/src/ir/localize.h +++ b/src/ir/localize.h @@ -110,7 +110,7 @@ struct ChildLocalizer { // Move the child out, and put an unreachable in its place (note that we // don't need an actual set here, as there is no value to set to a // local). - movedChildren.push_back(child); + sets.push_back(child); *childp = builder.makeUnreachable(); hasUnreachableChild = true; continue; @@ -121,7 +121,7 @@ struct ChildLocalizer { // (The only reason we still need them is that they may be needed for // validation, e.g. if one contains a break to a block that is the only // reason the block has type none.) - movedChildren.push_back(builder.makeDrop(child)); + sets.push_back(builder.makeDrop(child)); *childp = builder.makeUnreachable(); continue; } @@ -139,23 +139,23 @@ struct ChildLocalizer { } if (needLocal) { auto local = builder.addVar(func, child->type); - movedChildren.push_back(builder.makeLocalSet(local, child)); + sets.push_back(builder.makeLocalSet(local, child)); *childp = builder.makeLocalGet(local, child->type); } } } // Helper that gets a replacement for the parent: a block containing the - // movedChildren + the parent. This will not contain the parent if we don't - // need it (if it was never reached). + // sets + the parent. This will not contain the parent if we don't need it + // (if it was never reached). Expression* getReplacement() { - if (movedChildren.empty()) { + if (sets.empty()) { // Nothing to add. return parent; } auto* block = Builder(wasm).makeBlock(); - block->list.set(movedChildren); + block->list.set(sets); if (hasUnreachableChild) { // If there is an unreachable child then we do not need the parent at all, // and we know the type is unreachable. @@ -168,14 +168,11 @@ struct ChildLocalizer { return block; } - // The vector of moved children may be useful to some users directly, e.g. if - // all they care about are the children and not the parent. - std::vector movedChildren; - private: Expression* parent; Module& wasm; + std::vector sets; bool hasUnreachableChild = false; }; diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 37a6049b75d..8046380d5b9 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -18,7 +18,6 @@ #include "ir/gc-type-utils.h" #include "ir/iteration.h" #include "ir/local-structural-dominance.h" -#include "ir/localize.h" #include "ir/module-utils.h" #include "ir/subtypes.h" #include "ir/type-updating.h" @@ -989,23 +988,22 @@ void TranslateToFuzzReader::mutate(Function* func) { // just drop this expression (as a drop enables passes to think // they can remove more code, by marking the output "unused"). rep = parent.builder.makeSequence(parent.builder.makeDrop(curr), rep); - } else if (mode >= 66 && !Properties::isControlFlowStructure(curr) && - ChildIterator(curr).getNumChildren() > 0) { - // This is a normal (non-control-flow) expression with at least one - // child. "Interpose" between the children and this expression by - // keeping them and replacing the parent |curr|. We do this by - // generating sets of the children to locals, and keeping those. - auto sets = ChildLocalizer(curr, - getFunction(), - *getModule(), - PassOptions::getWithoutOptimization()) - .movedChildren; - auto* block = parent.builder.makeBlock(sets); - // TODO: Ideally the new expression |rep| could consume the children: - // all we need is for it to do local.get of the temp locals. - block->list.push_back(rep); - block->finalize(); - rep = block; + } else if (mode >= 66 && !Properties::isControlFlowStructure(curr)) { + ChildIterator children(curr); + if (children.getNumChildren() > 0) { + // This is a normal (non-control-flow) expression with at least one + // child. "Interpose" between the children and this expression by + // keeping them and replacing the parent |curr|. We do this by + // generating drops of the children. + auto* block = parent.builder.makeBlock(); + for (auto* child : children) { + block->list.push_back(parent.builder.makeDrop(child)); + } + // TODO: Ideally the new expression |rep| can consume the children. + block->list.push_back(rep); + block->finalize(); + rep = block; + } } replaceCurrent(rep); } From 4b4ddb4d0084b58ae411a95099cf899812a5717d Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 22 Mar 2024 15:00:28 -0700 Subject: [PATCH 06/14] work --- test/passes/fuzz_metrics_noprint.bin.txt | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/passes/fuzz_metrics_noprint.bin.txt b/test/passes/fuzz_metrics_noprint.bin.txt index e20068d52e6..7e272cccf94 100644 --- a/test/passes/fuzz_metrics_noprint.bin.txt +++ b/test/passes/fuzz_metrics_noprint.bin.txt @@ -8,21 +8,21 @@ total [table-data] : 7 [tables] : 1 [tags] : 0 - [total] : 12378 - [vars] : 79 - Binary : 892 - Block : 1963 + [total] : 12398 + [vars] : 64 + Binary : 893 + Block : 1965 Break : 478 Call : 288 CallIndirect : 131 - Const : 2052 - Drop : 89 - GlobalGet : 899 + Const : 2058 + Drop : 111 + GlobalGet : 901 GlobalSet : 714 If : 657 - Load : 246 + Load : 247 LocalGet : 1115 - LocalSet : 841 + LocalSet : 827 Loop : 317 Nop : 155 RefFunc : 7 From 832b8de4d5d1328adaac42c1d9d6ea0cde8f665b Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 25 Mar 2024 16:20:20 -0700 Subject: [PATCH 07/14] add ascii art --- src/tools/fuzzing/fuzzing.cpp | 40 +++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 8046380d5b9..d9439260d52 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -982,7 +982,29 @@ void TranslateToFuzzReader::mutate(Function* func) { if (mode < 33 && rep->type != Type::none) { // This has a non-none type. Replace the output, keeping the // expression and its children in a drop. This "interposes" between - // this expression and its parent. + // this expression and its parent, something like this: + // + // (D + // (A + // (B) + // (C) + // ) + // ) + //// + // => ;; keep A, replace it in the parent + // + // (D + // (block + // (drop + // (A + // (B) + // (C) + // ) + // ) + // (NEW) + // ) + // ) + // // TODO: Ideally the new expression here could consume |curr| as a // child, though if so we should still keep a decent chance to // just drop this expression (as a drop enables passes to think @@ -994,7 +1016,21 @@ void TranslateToFuzzReader::mutate(Function* func) { // This is a normal (non-control-flow) expression with at least one // child. "Interpose" between the children and this expression by // keeping them and replacing the parent |curr|. We do this by - // generating drops of the children. + // generating drops of the children, like this: + // + // (A + // (B) + // (C) + // ) + // + // => ;; keep children, replace A + // + // (block + // (drop (B)) + // (drop (C)) + // (NEW) + // ) + // auto* block = parent.builder.makeBlock(); for (auto* child : children) { block->list.push_back(parent.builder.makeDrop(child)); From d79db4339e37bddf862b980b9ba891a342a70ef4 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 25 Mar 2024 16:34:03 -0700 Subject: [PATCH 08/14] interpose more --- src/tools/fuzzing/fuzzing.cpp | 66 ++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index d9439260d52..ce33ba79d52 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -902,6 +902,20 @@ void TranslateToFuzzReader::recombine(Function* func) { modder.walk(func->body); } +// Given two expressions, try to replace one of the children of the first with +// the second. For example, given i32.add and an i32.const, the i32.const could +// be placed as either of the children of the i32.add. This tramples the +// existing content there. Returns true if we found a place. +static bool replaceChildWith(Expression* expr, Expression* with) { + for (auto*& child : ChildIterator(expr)) { + if (Type::isSubType(with->type, child->type)) { + child = with; + return true; + } + } + return false; +} + void TranslateToFuzzReader::mutate(Function* func) { // We want a 50% chance to not do this at all, and otherwise, we want to pick // a different frequency to do it in each function. That gives us more @@ -1005,16 +1019,36 @@ void TranslateToFuzzReader::mutate(Function* func) { // ) // ) // - // TODO: Ideally the new expression here could consume |curr| as a - // child, though if so we should still keep a decent chance to - // just drop this expression (as a drop enables passes to think - // they can remove more code, by marking the output "unused"). - rep = parent.builder.makeSequence(parent.builder.makeDrop(curr), rep); + // We also sometimes try to insert A as a child of NEW, so we actually + // interpose directly: + // + // (D + // (NEW + // (A + // (B) + // (C) + // ) + // ) + // ) + // + // We do not do that all the time, as inserting a drop is actually an + // important situation to test: the drop makes the output of A unused, + // which may let optimizations remove it. + if ((mode & 1) && replaceChildWith(rep, curr)) { + // We managed to replace one of the children with curr, and have + // nothing more to do. + } else { + // Drop curr and append. + rep = parent.builder.makeSequence(parent.builder.makeDrop(curr), + rep); + } } else if (mode >= 66 && !Properties::isControlFlowStructure(curr)) { ChildIterator children(curr); - if (children.getNumChildren() > 0) { + auto numChildren = children.getNumChildren(); + if (numChildren > 0 numChildren < 5) { // This is a normal (non-control-flow) expression with at least one - // child. "Interpose" between the children and this expression by + // child (and not an excessive amount of them; see the processing + // below). "Interpose" between the children and this expression by // keeping them and replacing the parent |curr|. We do this by // generating drops of the children, like this: // @@ -1033,12 +1067,20 @@ void TranslateToFuzzReader::mutate(Function* func) { // auto* block = parent.builder.makeBlock(); for (auto* child : children) { - block->list.push_back(parent.builder.makeDrop(child)); + // Only drop the child if we can't replace it as one of NEW's + // children. + if (!replaceChildWith(rep, child)) { + block->list.push_back(parent.builder.makeDrop(child)); + } + } + + if (!block->list.empty()) { + // We need the block, that is, we did not find a place for all the + // children. + block->list.push_back(rep); + block->finalize(); + rep = block; } - // TODO: Ideally the new expression |rep| can consume the children. - block->list.push_back(rep); - block->finalize(); - rep = block; } } replaceCurrent(rep); From 3eef11b1db338b07ed7f14e04e5bb7fc38573f08 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 25 Mar 2024 16:43:36 -0700 Subject: [PATCH 09/14] fix --- src/tools/fuzzing/fuzzing.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index ce33ba79d52..31ea2c088d3 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -1045,7 +1045,7 @@ void TranslateToFuzzReader::mutate(Function* func) { } else if (mode >= 66 && !Properties::isControlFlowStructure(curr)) { ChildIterator children(curr); auto numChildren = children.getNumChildren(); - if (numChildren > 0 numChildren < 5) { + if (numChildren > 0 && numChildren < 5) { // This is a normal (non-control-flow) expression with at least one // child (and not an excessive amount of them; see the processing // below). "Interpose" between the children and this expression by @@ -1068,7 +1068,8 @@ void TranslateToFuzzReader::mutate(Function* func) { auto* block = parent.builder.makeBlock(); for (auto* child : children) { // Only drop the child if we can't replace it as one of NEW's - // children. + // children. This does a linear scan of |rep| which is the reason + // for the above limit on the number of children. if (!replaceChildWith(rep, child)) { block->list.push_back(parent.builder.makeDrop(child)); } From 889030f329e05a92246bf661bf92040a67fc1f85 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 25 Mar 2024 16:44:18 -0700 Subject: [PATCH 10/14] update --- test/passes/fuzz_metrics_noprint.bin.txt | 42 ++++++++++++------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/test/passes/fuzz_metrics_noprint.bin.txt b/test/passes/fuzz_metrics_noprint.bin.txt index 7e272cccf94..f6af82a5ad0 100644 --- a/test/passes/fuzz_metrics_noprint.bin.txt +++ b/test/passes/fuzz_metrics_noprint.bin.txt @@ -8,27 +8,27 @@ total [table-data] : 7 [tables] : 1 [tags] : 0 - [total] : 12398 + [total] : 11685 [vars] : 64 - Binary : 893 - Block : 1965 - Break : 478 - Call : 288 - CallIndirect : 131 - Const : 2058 - Drop : 111 - GlobalGet : 901 - GlobalSet : 714 - If : 657 - Load : 247 - LocalGet : 1115 - LocalSet : 827 - Loop : 317 - Nop : 155 + Binary : 848 + Block : 1846 + Break : 456 + Call : 275 + CallIndirect : 117 + Const : 1952 + Drop : 86 + GlobalGet : 844 + GlobalSet : 679 + If : 625 + Load : 236 + LocalGet : 1050 + LocalSet : 764 + Loop : 301 + Nop : 143 RefFunc : 7 - Return : 107 - Select : 85 - Store : 113 + Return : 103 + Select : 77 + Store : 111 Switch : 3 - Unary : 883 - Unreachable : 343 + Unary : 835 + Unreachable : 327 From f7a9d10487fd432f5fdc45b18435429082255187 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 25 Mar 2024 16:45:43 -0700 Subject: [PATCH 11/14] format --- src/tools/fuzzing/fuzzing.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 31ea2c088d3..9f10d32d1f8 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -1039,8 +1039,8 @@ void TranslateToFuzzReader::mutate(Function* func) { // nothing more to do. } else { // Drop curr and append. - rep = parent.builder.makeSequence(parent.builder.makeDrop(curr), - rep); + rep = + parent.builder.makeSequence(parent.builder.makeDrop(curr), rep); } } else if (mode >= 66 && !Properties::isControlFlowStructure(curr)) { ChildIterator children(curr); From 0e635322b518370adecc9f824a13fa6e8f51d63e Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 25 Mar 2024 17:03:26 -0700 Subject: [PATCH 12/14] fix --- src/tools/fuzzing/fuzzing.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 9f10d32d1f8..4066b9f8b59 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -908,7 +908,9 @@ void TranslateToFuzzReader::recombine(Function* func) { // existing content there. Returns true if we found a place. static bool replaceChildWith(Expression* expr, Expression* with) { for (auto*& child : ChildIterator(expr)) { - if (Type::isSubType(with->type, child->type)) { + // To replace, we must have an appropriate type, and we cannot replace a + // Pop under any circumstances. + if (Type::isSubType(with->type, child->type) && !child->is()) { child = with; return true; } From bf9f0cde90bcb641a207be0d78b32d86992bbf0b Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 25 Mar 2024 17:07:23 -0700 Subject: [PATCH 13/14] fix --- src/tools/fuzzing/fuzzing.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 4066b9f8b59..6210c1d3d1a 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -910,7 +910,8 @@ static bool replaceChildWith(Expression* expr, Expression* with) { for (auto*& child : ChildIterator(expr)) { // To replace, we must have an appropriate type, and we cannot replace a // Pop under any circumstances. - if (Type::isSubType(with->type, child->type) && !child->is()) { + if (Type::isSubType(with->type, child->type) && + FindAll(child).list.empty()) { child = with; return true; } From cf50f766f93c4a04aae4cdd52a3b27cb5bc913d3 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 26 Mar 2024 10:05:55 -0700 Subject: [PATCH 14/14] TODOS --- src/tools/fuzzing/fuzzing.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp index 6210c1d3d1a..2c20f23ed19 100644 --- a/src/tools/fuzzing/fuzzing.cpp +++ b/src/tools/fuzzing/fuzzing.cpp @@ -900,6 +900,9 @@ void TranslateToFuzzReader::recombine(Function* func) { }; Modder modder(wasm, scanner, *this); modder.walk(func->body); + // TODO: A specific form of recombination we should perhaps do more often is + // to recombine among an expression's children, and in particular to + // reorder them. } // Given two expressions, try to replace one of the children of the first with @@ -1200,6 +1203,10 @@ void TranslateToFuzzReader::modifyInitialFunctions() { recombine(func); mutate(func); fixAfterChanges(func); + // TODO: This triad of functions appears in another place as well, and + // could be handled by a single function. That function could also + // decide to reorder recombine and mutate or even run more cycles of + // them. } } // Remove a start function - the fuzzing harness expects code to run only