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

Keep limiting in precompute as before: that is useful since that pass is run as part of normal compilation, and we want to avoid native stack limits on all platforms. Also that pass is not likely to find any pattern of size 50 of higher that it can't precompute as a sum of smaller pieces.

Restore the 250 limit from before for interpreting entire modules, as without that the fuzzer will sometimes hit the limit and cause a false positive.

Author: kripken

src/passes/Precompute.cpp

@@ -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
@@ -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

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<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;
+
+  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);
@@ -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]); }
 };
@@ -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
@@ -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) {
@@ -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);
     }
@@ -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) {
@@ -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;
@@ -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;
@@ -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;