Interpreter: Remove GlobalManager (#4486)

GlobalManager is another class that added complexity in the interpreter logic,
and did not help. In fact it hurts extensibility, as when one wants to extend the
interpreter one has another class to customize, and it is templated on the main
runner, so again as #4479 we end up with annoying template cycles.

This simply removes that class. That makes the interpreter code strictly
simpler. Applying that change to wasm-ctor-eval also ends up fixing a
pre-existing bug, so this PR gets testing through that.

The ctor-eval issue was that we did not extend the GlobalManager properly
in the past: we checked for accesses on imported globals there, but not in
the main class, i.e., not on global.get operations. Needing to do things in
two places is an example of the previous complexity. The fix is simply to
implement visitGlobalGet in one place, and remove all the GlobalManager
logic added in ctor-eval, which then gets a lot simpler as well.

The new imported-global-2.wast checks for that bug (a global.get of an
import should stop us from evalling). Existing tests cover the other cases,
like it being ok to read a non-imported global, etc. The existing test
indirect-call3.wast required a slight change: There was a global.get of
an imported global, which was ignored in the place it happened (an init
of an elem segment); the new code checks all global.gets, so it now
catches that.

8 files changed, 86 insertions, 114 deletions

diff --git a/src/tools/wasm-ctor-eval.cpp b/src/tools/wasm-ctor-eval.cpp
index 6dbff0c8f..271e96037 100644
--- a/src/tools/wasm-ctor-eval.cpp
+++ b/src/tools/wasm-ctor-eval.cpp
@@ -53,90 +53,23 @@ struct FailToEvalException {
 // the output.
 #define RECOMMENDATION "\n       recommendation: "
 
-// We do not have access to imported globals
-class EvallingGlobalManager {
-  // values of globals
-  std::map<Name, Literals> globals;
-
-  // globals that are dangerous to modify in the module
-  std::set<Name> dangerousGlobals;
-
-  // whether we are done adding new globals
-  bool sealed = false;
-
-public:
-  void addDangerous(Name name) { dangerousGlobals.insert(name); }
-
-  void seal() { sealed = true; }
-
-  Literals& operator[](Name name) {
-    if (dangerousGlobals.count(name) > 0) {
-      std::string extra;
-      if (name == "___dso_handle") {
-        extra = RECOMMENDATION
-          "build with -s NO_EXIT_RUNTIME=1 so that "
-          "calls to atexit that use ___dso_handle are not emitted";
-      }
-      throw FailToEvalException(
-        std::string(
-          "tried to access a dangerous (import-initialized) global: ") +
-        name.str + extra);
-    }
-    return globals[name];
-  }
-
-  struct Iterator {
-    Name first;
-    Literals second;
-    bool found;
-
-    Iterator() : found(false) {}
-    Iterator(Name name, Literals value)
-      : first(name), second(value), found(true) {}
-
-    bool operator==(const Iterator& other) {
-      return first == other.first && second == other.second &&
-             found == other.found;
-    }
-    bool operator!=(const Iterator& other) { return !(*this == other); }
-  };
-
-  Iterator find(Name name) {
-    if (globals.find(name) == globals.end()) {
-      return end();
-    }
-    return Iterator(name, globals[name]);
-  }
-
-  Iterator end() { return Iterator(); }
-
-  // Receives a module and applies the state of globals here into the globals
-  // in that module.
-  void applyToModule(Module& wasm) {
-    Builder builder(wasm);
-    for (const auto& [name, value] : globals) {
-      wasm.getGlobal(name)->init = builder.makeConstantExpression(value);
-    }
-  }
-};
-
-class EvallingModuleRunner
-  : public ModuleRunnerBase<EvallingGlobalManager, EvallingModuleRunner> {
+class EvallingModuleRunner : public ModuleRunnerBase<EvallingModuleRunner> {
 public:
   EvallingModuleRunner(
     Module& wasm,
     ExternalInterface* externalInterface,
     std::map<Name, std::shared_ptr<EvallingModuleRunner>> linkedInstances_ = {})
-    : ModuleRunnerBase(wasm, externalInterface, linkedInstances_) {
-    // if any global in the module has a non-const constructor, it is using a
-    // global import, which we don't have, and is illegal to use
-    ModuleUtils::iterDefinedGlobals(wasm, [&](Global* global) {
-      if (!global->init->is<Const>()) {
-        // this global is dangerously initialized by an import, so if it is
-        // used, we must fail
-        globals.addDangerous(global->name);
-      }
-    });
+    : ModuleRunnerBase(wasm, externalInterface, linkedInstances_) {}
+
+  Flow visitGlobalGet(GlobalGet* curr) {
+    // Error on reads of imported globals.
+    auto* global = wasm.getGlobal(curr->name);
+    if (global->imported()) {
+      throw FailToEvalException(std::string("read from imported global ") +
+                                global->module.str + "." + global->base.str);
+    }
+
+    return ModuleRunnerBase<EvallingModuleRunner>::visitGlobalGet(curr);
   }
 };
 
@@ -229,7 +162,7 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
       applyMemoryToModule();
     }
 
-    instance->globals.applyToModule(*wasm);
+    applyGlobalsToModule();
   }
 
   void init(Module& wasm_, EvallingModuleRunner& instance_) override {
@@ -237,7 +170,7 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
     instance = &instance_;
   }
 
-  void importGlobals(EvallingGlobalManager& globals, Module& wasm_) override {
+  void importGlobals(GlobalValueSet& globals, Module& wasm_) override {
     ModuleUtils::iterImportedGlobals(wasm_, [&](Global* global) {
       auto it = linkedInstances.find(global->module);
       if (it != linkedInstances.end()) {
@@ -487,6 +420,13 @@ private:
     // memory.
     segment.data = memory;
   }
+
+  void applyGlobalsToModule() {
+    Builder builder(*wasm);
+    for (const auto& [name, value] : instance->globals) {
+      wasm->getGlobal(name)->init = builder.makeConstantExpression(value);
+    }
+  }
 };
 
 // The outcome of evalling a ctor is one of three states:
@@ -695,9 +635,6 @@ void evalCtors(Module& wasm,
   try {
     // create an instance for evalling
     EvallingModuleRunner instance(wasm, &interface, linkedInstances);
-    // we should not add new globals from here on; as a result, using
-    // an imported global will fail, as it is missing and so looks new
-    instance.globals.seal();
     // go one by one, in order, until we fail
     // TODO: if we knew priorities, we could reorder?
     for (auto& ctor : ctors) {
diff --git a/src/wasm-interpreter.h b/src/wasm-interpreter.h
index 2a8df0111..c1d2d8674 100644
--- a/src/wasm-interpreter.h
+++ b/src/wasm-interpreter.h
@@ -2264,21 +2264,7 @@ public:
   }
 };
 
-// Execute an initializer expression of a global, data or element segment.
-// see: https://webassembly.org/docs/modules/#initializer-expression
-template<typename GlobalManager>
-class InitializerExpressionRunner
-  : public ExpressionRunner<InitializerExpressionRunner<GlobalManager>> {
-  GlobalManager& globals;
-
-public:
-  InitializerExpressionRunner(GlobalManager& globals, Index maxDepth)
-    : ExpressionRunner<InitializerExpressionRunner<GlobalManager>>(nullptr,
-                                                                   maxDepth),
-      globals(globals) {}
-
-  Flow visitGlobalGet(GlobalGet* curr) { return Flow(globals[curr->name]); }
-};
+using GlobalValueSet = std::map<Name, Literals>;
 
 //
 // A runner for a module. Each runner contains the information to execute the
@@ -2294,7 +2280,7 @@ public:
 // To call into the interpreter, use callExport.
 //
 
-template<typename GlobalManager, typename SubType>
+template<typename SubType>
 class ModuleRunnerBase : public ExpressionRunner<SubType> {
 public:
   //
@@ -2307,7 +2293,7 @@ public:
       std::map<Name, std::shared_ptr<SubType>> linkedInstances = {}) {}
     virtual ~ExternalInterface() = default;
     virtual void init(Module& wasm, SubType& instance) {}
-    virtual void importGlobals(GlobalManager& globals, Module& wasm) = 0;
+    virtual void importGlobals(GlobalValueSet& globals, Module& wasm) = 0;
     virtual Literals callImport(Function* import, Literals& arguments) = 0;
     virtual Literals callTable(Name tableName,
                                Index index,
@@ -2482,7 +2468,7 @@ public:
   Module& wasm;
 
   // Values of globals
-  GlobalManager globals;
+  GlobalValueSet globals;
 
   // Multivalue ABI support (see push/pop).
   std::vector<Literals> multiValues;
@@ -2499,10 +2485,7 @@ public:
     memorySize = wasm.memory.initial;
     // generate internal (non-imported) globals
     ModuleUtils::iterDefinedGlobals(wasm, [&](Global* global) {
-      globals[global->name] =
-        InitializerExpressionRunner<GlobalManager>(globals, maxDepth)
-          .visit(global->init)
-          .values;
+      globals[global->name] = self()->visit(global->init).values;
     });
 
     // initialize the rest of the external interface
@@ -3687,10 +3670,7 @@ protected:
   std::map<Name, std::shared_ptr<SubType>> linkedInstances;
 };
 
-// The default ModuleRunner uses a trivial global manager
-using TrivialGlobalManager = std::map<Name, Literals>;
-class ModuleRunner
-  : public ModuleRunnerBase<TrivialGlobalManager, ModuleRunner> {
+class ModuleRunner : public ModuleRunnerBase<ModuleRunner> {
 public:
   ModuleRunner(
     Module& wasm,
diff --git a/test/ctor-eval/imported-global-2.wast b/test/ctor-eval/imported-global-2.wast
new file mode 100644
index 000000000..e10dc7080
--- /dev/null
+++ b/test/ctor-eval/imported-global-2.wast
@@ -0,0 +1,31 @@
+(module
+  (memory 256 256)
+
+  ;; imports must not be used
+  (import "env" "imported" (global $imported i32))
+
+  (func "test1" (result i32)
+    (local $temp i32)
+
+    ;; this errors, and we never get to evalling the store after it
+    (local.set $temp
+      (global.get $imported)
+    )
+
+    (i32.store8
+      (i32.const 13)
+      (i32.const 115)
+    )
+
+    (local.get $temp)
+  )
+
+  (func "keepalive" (result i32)
+    (drop
+      (i32.load
+        (i32.const 13)
+      )
+    )
+    (global.get $imported)
+  )
+)
diff --git a/test/ctor-eval/imported-global-2.wast.ctors b/test/ctor-eval/imported-global-2.wast.ctors
new file mode 100644
index 000000000..a5bce3fd2
--- /dev/null
+++ b/test/ctor-eval/imported-global-2.wast.ctors
@@ -0,0 +1 @@
+test1
diff --git a/test/ctor-eval/imported-global-2.wast.out b/test/ctor-eval/imported-global-2.wast.out
new file mode 100644
index 000000000..ff71672a9
--- /dev/null
+++ b/test/ctor-eval/imported-global-2.wast.out
@@ -0,0 +1,26 @@
+(module
+ (type $none_=>_i32 (func (result i32)))
+ (import "env" "imported" (global $imported i32))
+ (memory $0 256 256)
+ (export "test1" (func $0))
+ (export "keepalive" (func $1))
+ (func $0 (result i32)
+  (local $temp i32)
+  (local.set $temp
+   (global.get $imported)
+  )
+  (i32.store8
+   (i32.const 13)
+   (i32.const 115)
+  )
+  (local.get $temp)
+ )
+ (func $1 (result i32)
+  (drop
+   (i32.load
+    (i32.const 13)
+   )
+  )
+  (global.get $imported)
+ )
+)
diff --git a/test/ctor-eval/imported-global.wast b/test/ctor-eval/imported-global.wast
index 92fda3fa0..20e56d2d1 100644
--- a/test/ctor-eval/imported-global.wast
+++ b/test/ctor-eval/imported-global.wast
@@ -6,7 +6,6 @@
   (export "test1" $test1)
   (global $mine (mut i32) (global.get $tempDoublePtr)) ;; BAD, if used
   (func $test1
-    (drop (global.get $mine))
     (i32.store8 (i32.const 13) (i32.const 115)) ;; we never get here.
   )
 )
diff --git a/test/ctor-eval/indirect-call3.wast b/test/ctor-eval/indirect-call3.wast
index 974ae7216..9c88e0f78 100644
--- a/test/ctor-eval/indirect-call3.wast
+++ b/test/ctor-eval/indirect-call3.wast
@@ -2,10 +2,9 @@
   (type $v (func))
   (memory 256 256)
   (data (i32.const 10) "waka waka waka waka waka")
-  (import "env" "tableBase" (global $tableBase i32))
   (import "env" "_abort" (func $_abort))
   (table 2 2 funcref)
-  (elem (global.get $tableBase) $_abort $call-indirect)
+  (elem (i32.const 0) $_abort $call-indirect)
   (export "test1" $test1)
   (func $test1
     (call_indirect (type $v) (i32.const 1)) ;; safe to call
diff --git a/test/ctor-eval/indirect-call3.wast.out b/test/ctor-eval/indirect-call3.wast.out
index 10914bca7..1c98857a6 100644
--- a/test/ctor-eval/indirect-call3.wast.out
+++ b/test/ctor-eval/indirect-call3.wast.out
@@ -1,6 +1,5 @@
 (module
  (type $v (func))
- (import "env" "tableBase" (global $tableBase i32))
  (import "env" "_abort" (func $_abort))
  (memory $0 256 256)
  (data (i32.const 10) "waka waka xaka waka waka\00\00\00\00\00\00C")