Refactor expression runner so it can be used via the C and JS APIs

src/binaryen-c.cpp

@@ -135,6 +135,7 @@ std::map<BinaryenGlobalRef, size_t> globals;
 std::map<BinaryenEventRef, size_t> events;
 std::map<BinaryenExportRef, size_t> exports;
 std::map<RelooperBlockRef, size_t> relooperBlocks;
+std::map<ExpressionRunnerRef, size_t> expressionRunners;
 
 static bool isBasicAPIType(BinaryenType type) {
   return type == BinaryenTypeAuto() || type <= Type::_last_value_type;
@@ -208,6 +209,26 @@ size_t noteExpression(BinaryenExpressionRef expression) {
   return id;
 }
 
+// We would normally use the size of `expressionRunners` as the next index, but
+// since we are going to delete runners the same address can become reused,
+// which would result in unpredictable sizes (indexes) due to undefined
+// behavior. Use a sequential id instead.
+static size_t nextExpressionRunnerId = 0;
+
+// Even though unlikely, it is possible that we are trying to use an id that is
+// still in use after wrapping around, which we must prevent.
+std::unordered_set<size_t> usedExpressionRunnerIds;
+
+size_t noteExpressionRunner(ExpressionRunnerRef runner) {
+  size_t id;
+  do {
+    id = nextExpressionRunnerId++;
+  } while (usedExpressionRunnerIds.find(id) != usedExpressionRunnerIds.end());
+  expressionRunners[runner] = id;
+  usedExpressionRunnerIds.insert(id);
+  return id;
+}
+
 std::string getTemp() {
   static size_t n = 0;
   return "t" + std::to_string(n++);
@@ -604,6 +625,7 @@ void BinaryenModuleDispose(BinaryenModuleRef module) {
     std::cout << "  events.clear();\n";
     std::cout << "  exports.clear();\n";
     std::cout << "  relooperBlocks.clear();\n";
+    std::cout << "  expressionRunners.clear();\n";
     types.clear();
     expressions.clear();
     functions.clear();
@@ -4954,6 +4976,22 @@ BinaryenExpressionRef RelooperRenderAndDispose(RelooperRef relooper,
 // ========= ExpressionRunner =========
 //
 
+namespace wasm {
+
+class CExpressionRunner final : public ExpressionRunner<CExpressionRunner> {
+public:
+  CExpressionRunner(Module* module,
+                    CExpressionRunner::Flags flags,
+                    Index maxDepth,
+                    Index maxLoopIterations)
+    : ExpressionRunner<CExpressionRunner>(
+        module, flags, maxDepth, maxLoopIterations) {}
+
+  void trap(const char* why) override { throw NonconstantException(); }
+};
+
+} // namespace wasm
+
 ExpressionRunnerFlags ExpressionRunnerFlagsDefault() {
   return CExpressionRunner::FlagValues::DEFAULT;
 }
@@ -4970,21 +5008,25 @@ ExpressionRunnerRef ExpressionRunnerCreate(BinaryenModuleRef module,
                                            ExpressionRunnerFlags flags,
                                            BinaryenIndex maxDepth,
                                            BinaryenIndex maxLoopIterations) {
-  if (tracing) {
-    std::cout << "  the_runner = ExpressionRunnerCreate(the_module, " << flags
-              << ", " << maxDepth << ", " << maxLoopIterations << ");\n";
-  }
   auto* wasm = (Module*)module;
-  return ExpressionRunnerRef(
+  auto* runner = ExpressionRunnerRef(
     new CExpressionRunner(wasm, flags, maxDepth, maxLoopIterations));
+  if (tracing) {
+    auto id = noteExpressionRunner(runner);
+    std::cout << "  expressionRunners[" << id
+              << "] = ExpressionRunnerCreate(the_module, " << flags << ", "
+              << maxDepth << ", " << maxLoopIterations << ");\n";
+  }
+  return runner;
 }
 
 int ExpressionRunnerSetLocalValue(ExpressionRunnerRef runner,
                                   BinaryenIndex index,
                                   BinaryenExpressionRef value) {
   if (tracing) {
-    std::cout << "  ExpressionRunnerSetLocalValue(the_runner, " << index
-              << ", expressions[" << expressions[value] << "]);\n";
+    std::cout << "  ExpressionRunnerSetLocalValue(expressionRunners["
+              << expressionRunners[runner] << "], " << index << ", expressions["
+              << expressions[value] << "]);\n";
   }
 
   auto* R = (CExpressionRunner*)runner;
@@ -5000,7 +5042,8 @@ int ExpressionRunnerSetGlobalValue(ExpressionRunnerRef runner,
                                    const char* name,
                                    BinaryenExpressionRef value) {
   if (tracing) {
-    std::cout << "  ExpressionRunnerSetGlobalValue(the_runner, ";
+    std::cout << "  ExpressionRunnerSetGlobalValue(expressionRunners["
+              << expressionRunners[runner] << "], ";
     traceNameOrNULL(name);
     std::cout << ", expressions[" << expressions[value] << "]);\n";
   }
@@ -5034,8 +5077,10 @@ ExpressionRunnerRunAndDispose(ExpressionRunnerRef runner,
     } else {
       std::cout << "  ";
     }
-    std::cout << "ExpressionRunnerRunAndDispose(the_runner, expressions["
-              << expressions[expr] << "]);\n";
+    auto id = expressionRunners[runner];
+    std::cout << "ExpressionRunnerRunAndDispose(expressionRunners[" << id
+              << "], expressions[" << expressions[expr] << "]);\n";
+    usedExpressionRunnerIds.erase(id);
   }
 
   delete R;
@@ -5062,9 +5107,9 @@ void BinaryenSetAPITracing(int on) {
                  "  std::map<size_t, BinaryenEventRef> events;\n"
                  "  std::map<size_t, BinaryenExportRef> exports;\n"
                  "  std::map<size_t, RelooperBlockRef> relooperBlocks;\n"
+                 "  std::map<size_t, ExpressionRunnerRef> expressionRunners;\n"
                  "  BinaryenModuleRef the_module = NULL;\n"
-                 "  RelooperRef the_relooper = NULL;\n"
-                 "  ExpressionRunnerRef the_runner = NULL;\n";
+                 "  RelooperRef the_relooper = NULL;\n";
   } else {
     std::cout << "  return 0;\n";
     std::cout << "}\n";

src/binaryen-c.h

@@ -1662,14 +1662,17 @@ typedef uint32_t ExpressionRunnerFlags;
 // side effects like those of a `local.tee`.
 BINARYEN_API ExpressionRunnerFlags ExpressionRunnerFlagsDefault();
 
-// Be very careful to preserve any side effects, like those of a `local.tee`,
-// for example when we are going to replace the expression afterwards.
+// Be very careful to preserve any side effects. For example, if we are
+// intending to replace the expression with a constant afterwards, even if we
+// can technically evaluate down to a constant, we still cannot replace the
+// expression if it also sets a local, which must be preserved in this scenario
+// so subsequent code keeps functioning.
 BINARYEN_API ExpressionRunnerFlags ExpressionRunnerFlagsPreserveSideeffects();
 
 // Traverse through function calls, attempting to compute their concrete value.
 // Must not be used in function-parallel scenarios, where the called function
-// might or might not have been optimized already to something we can traverse
-// successfully, in turn leading to non-deterministic behavior.
+// might be concurrently modified, leading to undefined behavior. Traversing
+// another function reuses all of this runner's flags.
 BINARYEN_API ExpressionRunnerFlags ExpressionRunnerFlagsTraverseCalls();
 
 // Creates an ExpressionRunner instance
@@ -1680,13 +1683,17 @@ ExpressionRunnerCreate(BinaryenModuleRef module,
                        BinaryenIndex maxLoopIterations);
 
 // Sets a known local value to use. Order matters if expressions have side
-// effects. Returns `true` if the expression actually evaluates to a constant.
+// effects. For example, if the expression also sets a local, this side effect
+// will also happen (not affected by any flags). Returns `true` if the
+// expression actually evaluates to a constant.
 BINARYEN_API int ExpressionRunnerSetLocalValue(ExpressionRunnerRef runner,
                                                BinaryenIndex index,
                                                BinaryenExpressionRef value);
 
 // Sets a known global value to use. Order matters if expressions have side
-// effects. Returns `true` if the expression actually evaluates to a constant.
+// effects. For example, if the expression also sets a local, this side effect
+// will also happen (not affected by any flags). Returns `true` if the
+// expression actually evaluates to a constant.
 BINARYEN_API int ExpressionRunnerSetGlobalValue(ExpressionRunnerRef runner,
                                                 const char* name,
                                                 BinaryenExpressionRef value);

src/passes/Precompute.cpp

@@ -74,9 +74,6 @@ class PrecomputingExpressionRunner
         MAX_LOOP_ITERATIONS),
       getValues(getValues) {}
 
-  struct NonconstantException {
-  }; // TODO: use a flow with a special name, as this is likely very slow
-
   Flow visitLocalGet(LocalGet* curr) {
     auto iter = getValues.find(curr);
     if (iter != getValues.end()) {

src/wasm-interpreter.h

@@ -155,14 +155,15 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
     // whether it computes down to a concrete value, where it is not necessary
     // to preserve side effects like those of a `local.tee`.
     DEFAULT = 0,
-    // Be very careful to preserve any side effects, like those of a
-    // `local.tee`, for example when we are going to replace the expression
-    // afterwards.
+    // Be very careful to preserve any side effects. For example, if we are
+    // intending to replace the expression with a constant afterwards, even if
+    // we can technically evaluate down to a constant, we still cannot replace
+    // the expression if it also sets a local, which must be preserved in this
+    // scenario so subsequent code keeps functioning.
     PRESERVE_SIDEEFFECTS = 1 << 0,
     // Traverse through function calls, attempting to compute their concrete
     // value. Must not be used in function-parallel scenarios, where the called
-    // function might or might not have been optimized already to something we
-    // can traverse successfully, in turn leading to non-deterministic behavior.
+    // function might be concurrently modified, leading to undefined behavior.
     TRAVERSE_CALLS = 1 << 1
   };
 
@@ -220,6 +221,9 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
     : module(module), flags(flags), maxDepth(maxDepth),
       maxLoopIterations(maxLoopIterations) {}
 
+  struct NonconstantException {
+  }; // TODO: use a flow with a special name, as this is likely very slow
+
   // Gets the module this runner is operating on.
   Module* getModule() { return module; }
 
@@ -1256,16 +1260,16 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
     NOTE_ENTER("LocalSet");
     NOTE_EVAL1(curr->index);
     if (!(flags & FlagValues::PRESERVE_SIDEEFFECTS)) {
-      // If we are evaluating and not replacing the expression, see if there is
-      // a value flowing through a tee.
+      // If we are evaluating and not replacing the expression, remember the
+      // constant value set, if any, and see if there is a value flowing through
+      // a tee.
       auto setFlow = visit(curr->value);
-      if (curr->type.isConcrete()) {
-        assert(curr->isTee());
-        return setFlow;
-      }
-      // Otherwise remember the constant value set, if any, for subsequent gets.
       if (!setFlow.breaking()) {
         setLocalValue(curr->index, setFlow.values);
+        if (curr->type.isConcrete()) {
+          assert(curr->isTee());
+          return setFlow;
+        }
         return Flow();
       }
     }
@@ -2532,22 +2536,6 @@ class ModuleInstance
     : ModuleInstanceBase(wasm, externalInterface) {}
 };
 
-// Expression runner exposed by the C-API
-class CExpressionRunner final : public ExpressionRunner<CExpressionRunner> {
-public:
-  CExpressionRunner(Module* module,
-                    CExpressionRunner::Flags flags,
-                    Index maxDepth,
-                    Index maxLoopIterations)
-    : ExpressionRunner<CExpressionRunner>(
-        module, flags, maxDepth, maxLoopIterations) {}
-
-  struct NonconstantException {
-  }; // TODO: use a flow with a special name, as this is likely very slow
-
-  void trap(const char* why) override { throw NonconstantException(); }
-};
-
 } // namespace wasm
 
 #endif // wasm_wasm_interpreter_h

test/binaryen.js/custom-section.js.txt

@@ -10,9 +10,9 @@ int main() {
   std::map<size_t, BinaryenEventRef> events;
   std::map<size_t, BinaryenExportRef> exports;
   std::map<size_t, RelooperBlockRef> relooperBlocks;
+  std::map<size_t, ExpressionRunnerRef> expressionRunners;
   BinaryenModuleRef the_module = NULL;
   RelooperRef the_relooper = NULL;
-  ExpressionRunnerRef the_runner = NULL;
   the_module = BinaryenModuleCreate();
   expressions[size_t(NULL)] = BinaryenExpressionRef(NULL);
   {

test/binaryen.js/expressionrunner.js

@@ -5,6 +5,15 @@ console.log("// ExpressionRunner.Flags.TraverseCalls = " + Flags.TraverseCalls);
 
 binaryen.setAPITracing(true);
 
+function assertDeepEqual(x, y) {
+  if (typeof x === "object") {
+    for (var i in x) assertDeepEqual(x[i], y[i]);
+    for (i in y) assertDeepEqual(x[i], y[i]);
+  } else {
+    assert(x === y);
+  }
+}
+
 var module = new binaryen.Module();
 module.addGlobal("aGlobal", binaryen.i32, true, module.i32.const(0));
 
@@ -16,7 +25,14 @@ var expr = runner.runAndDispose(
     module.i32.const(2)
   )
 );
-assert(JSON.stringify(binaryen.getExpressionInfo(expr)) === '{"id":14,"type":2,"value":3}');
+assertDeepEqual(
+  binaryen.getExpressionInfo(expr),
+  {
+    id: binaryen.ExpressionIds.Const,
+    type: binaryen.i32,
+    value: 3
+  }
+);
 
 // Should traverse control structures
 runner = new binaryen.ExpressionRunner(module);
@@ -30,7 +46,14 @@ expr = runner.runAndDispose(
     )
   ),
 );
-assert(JSON.stringify(binaryen.getExpressionInfo(expr)) === '{"id":14,"type":2,"value":4}');
+assertDeepEqual(
+  binaryen.getExpressionInfo(expr),
+  {
+    id: binaryen.ExpressionIds.Const,
+    type: binaryen.i32,
+    value: 4
+  }
+);
 
 // Should be unable to evaluate a local if not explicitly specified
 runner = new binaryen.ExpressionRunner(module);
@@ -57,7 +80,14 @@ expr = runner.runAndDispose(
     module.i32.const(1)
   )
 );
-assert(JSON.stringify(binaryen.getExpressionInfo(expr)) === '{"id":14,"type":2,"value":5}');
+assertDeepEqual(
+  binaryen.getExpressionInfo(expr),
+  {
+    id: binaryen.ExpressionIds.Const,
+    type: binaryen.i32,
+    value: 5
+  }
+);
 
 // Should preserve any side-effects if explicitly requested
 runner = new binaryen.ExpressionRunner(module, Flags.PreserveSideeffects);
@@ -83,7 +113,14 @@ expr = runner.runAndDispose(
     ], binaryen.i32)
   )
 );
-assert(JSON.stringify(binaryen.getExpressionInfo(expr)) === '{"id":14,"type":2,"value":6}');
+assertDeepEqual(
+  binaryen.getExpressionInfo(expr),
+  {
+    id: binaryen.ExpressionIds.Const,
+    type: binaryen.i32,
+    value: 6
+  }
+);
 
 // Should pick up explicitly preset values
 runner = new binaryen.ExpressionRunner(module, Flags.PreserveSideeffects);
@@ -95,7 +132,14 @@ expr = runner.runAndDispose(
     module.global.get("aGlobal", binaryen.i32)
   )
 );
-assert(JSON.stringify(binaryen.getExpressionInfo(expr)) === '{"id":14,"type":2,"value":7}');
+assertDeepEqual(
+  binaryen.getExpressionInfo(expr),
+  {
+    id: binaryen.ExpressionIds.Const,
+    type: binaryen.i32,
+    value: 7
+  }
+);
 
 // Should traverse into (simple) functions if requested
 runner = new binaryen.ExpressionRunner(module, Flags.TraverseCalls);
@@ -120,7 +164,14 @@ expr = runner.runAndDispose(
     module.local.get(0, binaryen.i32)
   )
 );
-assert(JSON.stringify(binaryen.getExpressionInfo(expr)) === '{"id":14,"type":2,"value":8}');
+assertDeepEqual(
+  binaryen.getExpressionInfo(expr),
+  {
+    id: binaryen.ExpressionIds.Const,
+    type: binaryen.i32,
+    value: 8
+  }
+);
 
 // Should not attempt to traverse into functions if not explicitly set
 runner = new binaryen.ExpressionRunner(module);
@@ -153,4 +204,5 @@ expr = runner.runAndDispose(
 );
 assert(expr === 0);
 
+module.dispose();
 binaryen.setAPITracing(false);