From f6a81c681d73a116db3b9c7da6d55f36d5979b72 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Sat, 29 Jun 2019 19:45:38 -0700 Subject: [PATCH 1/5] wip --- src/passes/Precompute.cpp | 11 ++++++++++- src/wasm-interpreter.h | 31 ++++++++++++++++++------------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/passes/Precompute.cpp b/src/passes/Precompute.cpp index 2e8c8fe31d1..87c32a6c8fd 100644 --- a/src/passes/Precompute.cpp +++ b/src/passes/Precompute.cpp @@ -41,6 +41,14 @@ namespace wasm { static const Name NOTPRECOMPUTABLE_FLOW("Binaryen|notprecomputable"); +// Limit evaluation depth for 2 reasons: first, it is highly unlikely +// that we can do anything useful to precompute a hugely nested expression +// (we should succed at smaller parts of it first). Second, a low limit is +// helpful to avoid platform differences in native stack sizes. +enum { + MAX_DEPTH = 50 +}; + typedef std::unordered_map GetValues; // Precomputes an expression. Errors if we hit anything that can't be @@ -63,7 +71,8 @@ class PrecomputingExpressionRunner PrecomputingExpressionRunner(Module* module, GetValues& getValues, bool replaceExpression) - : module(module), getValues(getValues), + : ExpressionRunner(MAX_DEPTH), + module(module), getValues(getValues), replaceExpression(replaceExpression) {} struct NonstandaloneException { diff --git a/src/wasm-interpreter.h b/src/wasm-interpreter.h index ca9b35babc3..85e05cfa688 100644 --- a/src/wasm-interpreter.h +++ b/src/wasm-interpreter.h @@ -45,8 +45,6 @@ using namespace cashew; extern Name WASM, RETURN_FLOW; -enum { maxInterpreterDepth = 50 }; - // Stuff that flows around during executing expressions: a literal, or a change // in control flow. class Flow { @@ -129,13 +127,16 @@ class Indenter { template class ExpressionRunner : public OverriddenVisitor { protected: - // Keep a record of call depth, to guard against excessive recursion. - size_t depth = 0; + Index maxDepth; + + Index depth = 0; public: + ExpressionRunner(Index maxDepth) : maxDepth(maxDepth) {} + Flow visit(Expression* curr) { depth++; - if (depth > maxInterpreterDepth) { + if (depth > maxDepth) { trap("interpreter recursion limit"); } auto ret = OverriddenVisitor::visit(curr); @@ -1056,7 +1057,7 @@ class ConstantExpressionRunner GlobalManager& globals; public: - ConstantExpressionRunner(GlobalManager& globals) : globals(globals) {} + ConstantExpressionRunner(GlobalManager& globals, Index maxDepth) : ExpressionRunner>(maxDepth), globals(globals) {} Flow visitGlobalGet(GlobalGet* curr) { return Flow(globals[curr->name]); } }; @@ -1232,7 +1233,7 @@ template class ModuleInstanceBase { memorySize = wasm.memory.initial; // generate internal (non-imported) globals ModuleUtils::iterDefinedGlobals(wasm, [&](Global* global) { - globals[global->name] = ConstantExpressionRunner(globals) + globals[global->name] = ConstantExpressionRunner(globals, maxDepth) .visit(global->init) .value; }); @@ -1297,7 +1298,7 @@ template class ModuleInstanceBase { void initializeTableContents() { for (auto& segment : wasm.table.segments) { Address offset = - (uint32_t)ConstantExpressionRunner(globals) + (uint32_t)ConstantExpressionRunner(globals, maxDepth) .visit(segment.offset) .value.geti32(); if (offset + segment.data.size() > wasm.table.initial) { @@ -1340,7 +1341,7 @@ template class ModuleInstanceBase { // the memory.init and data.drop instructions. Function dummyFunc; FunctionScope dummyScope(&dummyFunc, {}); - RuntimeExpressionRunner runner(*this, dummyScope); + RuntimeExpressionRunner runner(*this, dummyScope, maxDepth); runner.visit(&init); runner.visit(&drop); } @@ -1387,8 +1388,8 @@ template class ModuleInstanceBase { FunctionScope& scope; public: - RuntimeExpressionRunner(ModuleInstanceBase& instance, FunctionScope& scope) - : instance(instance), scope(scope) {} + RuntimeExpressionRunner(ModuleInstanceBase& instance, FunctionScope& scope, Index maxDepth) + : ExpressionRunner(maxDepth), instance(instance), scope(scope) {} Flow generateArguments(const ExpressionList& operands, LiteralList& arguments) { @@ -1799,7 +1800,7 @@ template class ModuleInstanceBase { // Internal function call. Must be public so that callTable implementations // can use it (refactor?) Literal callFunctionInternal(Name name, const LiteralList& arguments) { - if (callDepth > maxInterpreterDepth) { + if (callDepth > maxDepth) { externalInterface->trap("stack limit"); } auto previousCallDepth = callDepth; @@ -1818,7 +1819,7 @@ template class ModuleInstanceBase { } #endif - Flow flow = RuntimeExpressionRunner(*this, scope).visit(function->body); + Flow flow = RuntimeExpressionRunner(*this, scope, maxDepth).visit(function->body); // cannot still be breaking, it means we missed our stop assert(!flow.breaking() || flow.breakTo == RETURN_FLOW); Literal ret = flow.value; @@ -1842,6 +1843,10 @@ template class ModuleInstanceBase { protected: Address memorySize; // in pages + enum { + maxDepth = 1024 + }; + void trapIfGt(uint64_t lhs, uint64_t rhs, const char* msg) { if (lhs > rhs) { std::stringstream ss; From 1cfff103be5eedb46c465d0f28db2cb321ff17ce Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Sat, 29 Jun 2019 20:13:57 -0700 Subject: [PATCH 2/5] fix --- src/wasm-interpreter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wasm-interpreter.h b/src/wasm-interpreter.h index 85e05cfa688..093e912bd12 100644 --- a/src/wasm-interpreter.h +++ b/src/wasm-interpreter.h @@ -1844,7 +1844,7 @@ template class ModuleInstanceBase { Address memorySize; // in pages enum { - maxDepth = 1024 + maxDepth = 250 }; void trapIfGt(uint64_t lhs, uint64_t rhs, const char* msg) { From b43302175629a737e7a1d876d84b51d3fff329b6 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Sun, 30 Jun 2019 08:36:59 -0700 Subject: [PATCH 3/5] format --- src/passes/Precompute.cpp | 9 +++------ src/wasm-interpreter.h | 25 +++++++++++++++---------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/passes/Precompute.cpp b/src/passes/Precompute.cpp index 87c32a6c8fd..6e241863c54 100644 --- a/src/passes/Precompute.cpp +++ b/src/passes/Precompute.cpp @@ -45,9 +45,7 @@ static const Name NOTPRECOMPUTABLE_FLOW("Binaryen|notprecomputable"); // that we can do anything useful to precompute a hugely nested expression // (we should succed at smaller parts of it first). Second, a low limit is // helpful to avoid platform differences in native stack sizes. -enum { - MAX_DEPTH = 50 -}; +enum { MAX_DEPTH = 50 }; typedef std::unordered_map GetValues; @@ -71,9 +69,8 @@ class PrecomputingExpressionRunner PrecomputingExpressionRunner(Module* module, GetValues& getValues, bool replaceExpression) - : ExpressionRunner(MAX_DEPTH), - module(module), getValues(getValues), - replaceExpression(replaceExpression) {} + : ExpressionRunner(MAX_DEPTH), module(module), + getValues(getValues), replaceExpression(replaceExpression) {} struct NonstandaloneException { }; // TODO: use a flow with a special name, as this is likely very slow diff --git a/src/wasm-interpreter.h b/src/wasm-interpreter.h index 093e912bd12..b61f32b0ed8 100644 --- a/src/wasm-interpreter.h +++ b/src/wasm-interpreter.h @@ -1057,7 +1057,9 @@ class ConstantExpressionRunner GlobalManager& globals; public: - ConstantExpressionRunner(GlobalManager& globals, Index maxDepth) : ExpressionRunner>(maxDepth), globals(globals) {} + ConstantExpressionRunner(GlobalManager& globals, Index maxDepth) + : ExpressionRunner>(maxDepth), + globals(globals) {} Flow visitGlobalGet(GlobalGet* curr) { return Flow(globals[curr->name]); } }; @@ -1233,9 +1235,10 @@ template class ModuleInstanceBase { memorySize = wasm.memory.initial; // generate internal (non-imported) globals ModuleUtils::iterDefinedGlobals(wasm, [&](Global* global) { - globals[global->name] = ConstantExpressionRunner(globals, maxDepth) - .visit(global->init) - .value; + globals[global->name] = + ConstantExpressionRunner(globals, maxDepth) + .visit(global->init) + .value; }); // initialize the rest of the external interface @@ -1388,8 +1391,11 @@ template class ModuleInstanceBase { FunctionScope& scope; public: - RuntimeExpressionRunner(ModuleInstanceBase& instance, FunctionScope& scope, Index maxDepth) - : ExpressionRunner(maxDepth), instance(instance), scope(scope) {} + RuntimeExpressionRunner(ModuleInstanceBase& instance, + FunctionScope& scope, + Index maxDepth) + : ExpressionRunner(maxDepth), instance(instance), + scope(scope) {} Flow generateArguments(const ExpressionList& operands, LiteralList& arguments) { @@ -1819,7 +1825,8 @@ template class ModuleInstanceBase { } #endif - Flow flow = RuntimeExpressionRunner(*this, scope, maxDepth).visit(function->body); + Flow flow = + RuntimeExpressionRunner(*this, scope, maxDepth).visit(function->body); // cannot still be breaking, it means we missed our stop assert(!flow.breaking() || flow.breakTo == RETURN_FLOW); Literal ret = flow.value; @@ -1843,9 +1850,7 @@ template class ModuleInstanceBase { protected: Address memorySize; // in pages - enum { - maxDepth = 250 - }; + enum { maxDepth = 250 }; void trapIfGt(uint64_t lhs, uint64_t rhs, const char* msg) { if (lhs > rhs) { From d9ab9b6cc7fc5aac5c58eef8094d13024aa98ea8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 1 Jul 2019 13:56:54 -0700 Subject: [PATCH 4/5] review feedback --- src/passes/Precompute.cpp | 2 +- src/wasm-interpreter.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/passes/Precompute.cpp b/src/passes/Precompute.cpp index 6e241863c54..a9ea0b5525f 100644 --- a/src/passes/Precompute.cpp +++ b/src/passes/Precompute.cpp @@ -45,7 +45,7 @@ static const Name NOTPRECOMPUTABLE_FLOW("Binaryen|notprecomputable"); // that we can do anything useful to precompute a hugely nested expression // (we should succed at smaller parts of it first). Second, a low limit is // helpful to avoid platform differences in native stack sizes. -enum { MAX_DEPTH = 50 }; +static const Index maxDepth = 50; typedef std::unordered_map GetValues; diff --git a/src/wasm-interpreter.h b/src/wasm-interpreter.h index b61f32b0ed8..b111e755d17 100644 --- a/src/wasm-interpreter.h +++ b/src/wasm-interpreter.h @@ -1850,7 +1850,7 @@ template class ModuleInstanceBase { protected: Address memorySize; // in pages - enum { maxDepth = 250 }; + static const Index maxDepth = 250; void trapIfGt(uint64_t lhs, uint64_t rhs, const char* msg) { if (lhs > rhs) { From 8a7f43cf7a67a1b107207ca6f6e9c150da15c5fd Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 1 Jul 2019 17:47:06 -0700 Subject: [PATCH 5/5] fix --- src/passes/Precompute.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/passes/Precompute.cpp b/src/passes/Precompute.cpp index a9ea0b5525f..a7edb54c9c3 100644 --- a/src/passes/Precompute.cpp +++ b/src/passes/Precompute.cpp @@ -45,7 +45,7 @@ static const Name NOTPRECOMPUTABLE_FLOW("Binaryen|notprecomputable"); // that we can do anything useful to precompute a hugely nested expression // (we should succed at smaller parts of it first). Second, a low limit is // helpful to avoid platform differences in native stack sizes. -static const Index maxDepth = 50; +static const Index MAX_DEPTH = 50; typedef std::unordered_map GetValues;