Skip to content

Limit interpreter depth in precompute, but not when running whole modules #2191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kripken merged 6 commits into from
Jul 2, 2019
Merged

Limit interpreter depth in precompute, but not when running whole modules #2191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f6a81c6
wip
kripken Jun 30, 2019
1cfff10
fix
kripken Jun 30, 2019
b433021
format
kripken Jun 30, 2019
d9ab9b6
review feedback
kripken Jul 1, 2019
de31627
Merge remote-tracking branch 'origin/master' into a5
kripken Jul 1, 2019
8a7f43c
fix
kripken Jul 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/passes/Precompute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ 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.
static const Index MAX_DEPTH = 50;

typedef std::unordered_map<LocalGet*, Literal> GetValues;

// Precomputes an expression. Errors if we hit anything that can't be
Expand All @@ -63,8 +69,8 @@ class PrecomputingExpressionRunner
PrecomputingExpressionRunner(Module* module,
GetValues& getValues,
bool replaceExpression)
: module(module), getValues(getValues),
replaceExpression(replaceExpression) {}
: ExpressionRunner<PrecomputingExpressionRunner>(MAX_DEPTH), module(module),
getValues(getValues), replaceExpression(replaceExpression) {}

struct NonstandaloneException {
}; // TODO: use a flow with a special name, as this is likely very slow
Expand Down
40 changes: 25 additions & 15 deletions src/wasm-interpreter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -129,13 +127,16 @@ class Indenter {
template<typename SubType>
class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
protected:
// Keep a record of call depth, to guard against excessive recursion.
size_t depth = 0;
Index maxDepth;

Copy link
Copy Markdown
Member

@tlively tlively Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this whitespace looks unnecessary.

Copy link
Copy Markdown
Member Author

@kripken kripken Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's sort of separating the input values from the internal state values. Is there a clearer way to do that? I could use another protected: block lower down I guess.

Copy link
Copy Markdown
Member

@tlively tlively Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this newline seems fine if it's there intentionally 👍

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<SubType, Flow>::visit(curr);
Expand Down Expand Up @@ -1056,7 +1057,9 @@ class ConstantExpressionRunner
GlobalManager& globals;

public:
ConstantExpressionRunner(GlobalManager& globals) : globals(globals) {}
ConstantExpressionRunner(GlobalManager& globals, Index maxDepth)
: ExpressionRunner<ConstantExpressionRunner<GlobalManager>>(maxDepth),
globals(globals) {}

Flow visitGlobalGet(GlobalGet* curr) { return Flow(globals[curr->name]); }
};
Expand Down Expand Up @@ -1232,9 +1235,10 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
memorySize = wasm.memory.initial;
// generate internal (non-imported) globals
ModuleUtils::iterDefinedGlobals(wasm, [&](Global* global) {
globals[global->name] = ConstantExpressionRunner<GlobalManager>(globals)
.visit(global->init)
.value;
globals[global->name] =
ConstantExpressionRunner<GlobalManager>(globals, maxDepth)
.visit(global->init)
.value;
});

// initialize the rest of the external interface
Expand Down Expand Up @@ -1297,7 +1301,7 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
void initializeTableContents() {
for (auto& segment : wasm.table.segments) {
Address offset =
(uint32_t)ConstantExpressionRunner<GlobalManager>(globals)
(uint32_t)ConstantExpressionRunner<GlobalManager>(globals, maxDepth)
.visit(segment.offset)
.value.geti32();
if (offset + segment.data.size() > wasm.table.initial) {
Expand Down Expand Up @@ -1340,7 +1344,7 @@ template<typename GlobalManager, typename SubType> 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);
}
Expand Down Expand Up @@ -1387,8 +1391,11 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
FunctionScope& scope;

public:
RuntimeExpressionRunner(ModuleInstanceBase& instance, FunctionScope& scope)
: instance(instance), scope(scope) {}
RuntimeExpressionRunner(ModuleInstanceBase& instance,
FunctionScope& scope,
Index maxDepth)
: ExpressionRunner<RuntimeExpressionRunner>(maxDepth), instance(instance),
scope(scope) {}

Flow generateArguments(const ExpressionList& operands,
LiteralList& arguments) {
Expand Down Expand Up @@ -1788,7 +1795,7 @@ template<typename GlobalManager, typename SubType> 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;
Expand All @@ -1807,7 +1814,8 @@ template<typename GlobalManager, typename SubType> 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;
Expand All @@ -1831,6 +1839,8 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
protected:
Address memorySize; // in pages

static const Index maxDepth = 250;

void trapIfGt(uint64_t lhs, uint64_t rhs, const char* msg) {
if (lhs > rhs) {
std::stringstream ss;
Expand Down