[NFC-ish] Refactor away callTable (#7776)

Historically, wasm only had call_indirect, and we put callTable as a
method on the interpreter's ExternalInterface. Later, wasm added
table.get etc., and we added tableLoad to that interface. Given that,
we don't really need callTable: It is simpler for e.g. visitCallIndirect
to use tableLoad + do a call of the loaded reference.

This removes a bunch of duplicate code around the codebase
(implementers of callTable), and makes working on visitCallIndirect
simpler (which I will need for stack switching).

Some minor test changes happen here, mostly that the trap
message is slightly different in some cases. The one interesting
change, which makes this not NFC, is that a TODO is now
complete in test/lit/ctor-eval/return_call.wast

Author: kripken

src/shell-interface.h

@@ -152,53 +152,6 @@ struct ShellExternalInterface : ModuleRunner::ExternalInterface {
             << import->name.str;
   }
 
-  Literals callTable(Name tableName,
-                     Address index,
-                     HeapType sig,
-                     Literals& arguments,
-                     Type results,
-                     ModuleRunner& instance) override {
-
-    auto it = tables.find(tableName);
-    if (it == tables.end()) {
-      trap("callTable on non-existing table");
-    }
-
-    auto& table = it->second;
-    if (index >= table.size()) {
-      trap("callTable overflow");
-    }
-    Function* func = nullptr;
-    if (table[index].isFunction() && !table[index].isNull()) {
-      func = instance.wasm.getFunctionOrNull(table[index].getFunc());
-    }
-    if (!func) {
-      trap("uninitialized table element");
-    }
-    if (sig != func->type) {
-      trap("callIndirect: function types don't match");
-    }
-    if (func->getParams().size() != arguments.size()) {
-      trap("callIndirect: bad # of arguments");
-    }
-    size_t i = 0;
-    for (const auto& param : func->getParams()) {
-      if (!Type::isSubType(arguments[i++].type, param)) {
-        trap("callIndirect: bad argument type");
-      }
-    }
-    if (func->getResults() != results) {
-      trap("callIndirect: bad result type");
-    }
-    if (func->imported()) {
-      return callImport(func, arguments);
-    } else {
-      auto flow = instance.callFunction(func->name, arguments);
-      assert(!flow.suspendTag); // TODO: support stack switching on calls
-      return flow.values;
-    }
-  }
-
   int8_t load8s(Address addr, Name memoryName) override {
     auto it = memories.find(memoryName);
     assert(it != memories.end());

src/tools/wasm-ctor-eval.cpp

@@ -86,6 +86,12 @@ class EvallingModuleRunner : public ModuleRunnerBase<EvallingModuleRunner> {
 
     return ModuleRunnerBase<EvallingModuleRunner>::visitGlobalGet(curr);
   }
+
+  Flow visitTableGet(TableGet* curr) {
+    // We support tableLoad, below, so that call_indirect works (it calls it
+    // internally), but we want to disable table.get for now.
+    throw FailToEvalException("TODO: table.get");
+  }
 };
 
 // Build an artificial `env` module based on a module's imports, so that the
@@ -289,23 +295,15 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
   }
 
   // We assume the table is not modified FIXME
-  Literals callTable(Name tableName,
-                     Address index,
-                     HeapType sig,
-                     Literals& arguments,
-                     Type result,
-                     EvallingModuleRunner& instance) override {
-
-    std::unordered_map<wasm::Name, std::vector<wasm::Name>>::iterator it;
-
+  Literal tableLoad(Name tableName, Address index) override {
     auto* table = wasm->getTableOrNull(tableName);
     if (!table) {
-      throw FailToEvalException("callTable on non-existing table");
+      throw FailToEvalException("tableLoad on non-existing table");
     }
 
-    // Look through the segments and find the function. Segments can overlap,
+    // Look through the segments and find the value. Segments can overlap,
     // so we want the last one.
-    Name targetFunc;
+    Expression* value = nullptr;
     for (auto& segment : wasm->elementSegments) {
       if (segment->table != tableName) {
         continue;
@@ -326,51 +324,25 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
       }
       auto end = start + segment->data.size();
       if (start <= index && index < end) {
-        auto entry = segment->data[index - start];
-        if (auto* get = entry->dynCast<RefFunc>()) {
-          targetFunc = get->func;
-        } else {
-          throw FailToEvalException(
-            std::string("callTable on uninitialized entry"));
-        }
+        value = segment->data[index - start];
       }
     }
 
-    if (!targetFunc.is()) {
-      throw FailToEvalException(
-        std::string("callTable on index not found in static segments: ") +
-        std::to_string(index));
+    if (!value) {
+      // No segment had a value for this.
+      return Literal::makeNull(HeapTypes::func);
     }
-
-    // If this is one of our functions, we can call it; if it was
-    // imported, fail.
-    auto* func = wasm->getFunction(targetFunc);
-    if (func->type != sig) {
-      throw FailToEvalException(std::string("callTable signature mismatch: ") +
-                                targetFunc.toString());
-    }
-    if (!func->imported()) {
-      auto flow = instance.callFunction(targetFunc, arguments);
-      if (flow.suspendTag) {
-        throw FailToEvalException("unhandled suspend");
-      }
-      return flow.values;
-    } else {
-      throw FailToEvalException(
-        std::string("callTable on imported function: ") +
-        targetFunc.toString());
+    if (!Properties::isConstantExpression(value)) {
+      throw FailToEvalException("tableLoad of non-literal");
     }
+    return Properties::getLiteral(value);
   }
 
   Index tableSize(Name tableName) override {
-    // See callTable above, we assume the table is not modified FIXME
+    // See tableLoad above, we assume the table is not modified FIXME
     return wasm->getTableOrNull(tableName)->initial;
   }
 
-  Literal tableLoad(Name tableName, Address index) override {
-    throw FailToEvalException("table.get: TODO");
-  }
-
   // called during initialization
   void
   tableStore(Name tableName, Address index, const Literal& value) override {

src/wasm-interpreter.h

@@ -3006,12 +3006,6 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
     virtual void importGlobals(GlobalValueSet& globals, Module& wasm) = 0;
     virtual Literals callImport(Function* import,
                                 const Literals& arguments) = 0;
-    virtual Literals callTable(Name tableName,
-                               Address index,
-                               HeapType sig,
-                               Literals& arguments,
-                               Type result,
-                               SubType& instance) = 0;
     virtual bool growMemory(Name name, Address oldSize, Address newSize) = 0;
     virtual bool growTable(Name name,
                            const Literal& value,
@@ -3560,23 +3554,34 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
 
     auto index = target.getSingleValue().getUnsigned();
     auto info = getTableInstanceInfo(curr->table);
+    auto funcref = info.interface()->tableLoad(info.name, index);
 
     if (curr->isReturn) {
       // Return calls are represented by their arguments followed by a reference
       // to the function to be called.
-      auto funcref = info.interface()->tableLoad(info.name, index);
       if (!Type::isSubType(funcref.type, Type(curr->heapType, NonNullable))) {
         trap("cast failure in call_indirect");
       }
       arguments.push_back(funcref);
       return Flow(RETURN_CALL_FLOW, std::move(arguments));
     }
 
+    if (funcref.isNull()) {
+      trap("null target in call_indirect");
+    }
+    if (!funcref.isFunction()) {
+      trap("non-function target in call_indirect");
+    }
+
+    auto* func = self()->getModule()->getFunction(funcref.getFunc());
+    if (!HeapType::isSubType(func->type, curr->heapType)) {
+      trap("callIndirect: non-subtype");
+    }
+
 #if WASM_INTERPRETER_DEBUG
     std::cout << self()->indent() << "(calling table)\n";
 #endif
-    Flow ret = info.interface()->callTable(
-      info.name, index, curr->heapType, arguments, curr->type, *self());
+    Flow ret = callFunction(funcref.getFunc(), arguments);
 #if WASM_INTERPRETER_DEBUG
     std::cout << self()->indent() << "(returned to " << scope->function->name
               << ")\n";

test/lit/ctor-eval/return_call.wast

@@ -60,7 +60,6 @@
 ;; CHECK-NEXT: )
 (module
   ;; Basic return call indirect
-  ;; TODO: Implement `tableLoad` to make this test work.
 
   (import "env" "import" (func $import))
 
@@ -69,10 +68,9 @@
   ;; CHECK:      (global $g1 (mut i32) (i32.const 1))
   (global $g1 (export "g1") (mut i32) (i32.const 0))
 
-  ;; CHECK:      (global $g2 (mut i32) (i32.const 0))
+  ;; CHECK:      (global $g2 (mut i32) (i32.const 2))
   (global $g2 (export "g2") (mut i32) (i32.const 0))
 
-  ;; CHECK:      (table $t 1 1 funcref)
   (table $t funcref (elem $test2))
 
   (func $test (export "test")
@@ -86,30 +84,21 @@
     (call $import)
   )
 
-  ;; CHECK:      (elem $implicit-elem (i32.const 0) $test2)
-
-  ;; CHECK:      (export "g1" (global $g1))
-
-  ;; CHECK:      (export "g2" (global $g2))
-
-  ;; CHECK:      (export "test" (func $test_3))
-
-  ;; CHECK:      (func $test2 (type $0)
-  ;; CHECK-NEXT:  (global.set $g2
-  ;; CHECK-NEXT:   (i32.const 2)
-  ;; CHECK-NEXT:  )
-  ;; CHECK-NEXT: )
   (func $test2
     (global.set $g2
       (i32.const 2)
     )
   )
 )
 
+;; CHECK:      (export "g1" (global $g1))
+
+;; CHECK:      (export "g2" (global $g2))
+
+;; CHECK:      (export "test" (func $test_3))
+
 ;; CHECK:      (func $test_3 (type $0)
-;; CHECK-NEXT:  (return_call_indirect $t (type $0)
-;; CHECK-NEXT:   (i32.const 0)
-;; CHECK-NEXT:  )
+;; CHECK-NEXT:  (nop)
 ;; CHECK-NEXT: )
 (module
   ;; Basic return call ref

test/lit/exec/table.fill.wast

@@ -22,7 +22,7 @@
   )
  )
  ;; CHECK:      [fuzz-exec] calling call
- ;; CHECK-NEXT: [trap uninitialized table element]
+ ;; CHECK-NEXT: [trap null target in call_indirect]
  (func $call (export "call") (result i32)
   ;; Nothing was written, so this traps.
   (call_indirect $table (type $i32)

test/lit/exec/table64.wast

@@ -35,7 +35,7 @@
  )
 
  ;; CHECK:      [fuzz-exec] calling oob
- ;; CHECK-NEXT: [trap callTable overflow]
+ ;; CHECK-NEXT: [trap out of bounds table access]
  (func $oob (export "oob") (result i32)
   ;; This call traps on oob.
   (call_indirect (type $i32)
@@ -44,7 +44,7 @@
  )
 
  ;; CHECK:      [fuzz-exec] calling oob-huge
- ;; CHECK-NEXT: [trap callTable overflow]
+ ;; CHECK-NEXT: [trap out of bounds table access]
  (func $oob-huge (export "oob-huge") (result i32)
   ;; This call traps on oob with a value over 32 bits, 2**32 + 1, which if we
   ;; truncated to 32 bits, would seem in bounds, and end up calling a valid
@@ -58,7 +58,7 @@
  )
 
  ;; CHECK:      [fuzz-exec] calling null
- ;; CHECK-NEXT: [trap uninitialized table element]
+ ;; CHECK-NEXT: [trap null target in call_indirect]
  (func $null (export "null") (result i32)
   ;; This call traps on null
   (call_indirect (type $i32)
@@ -74,13 +74,13 @@
 ;; CHECK-NEXT: [fuzz-exec] note result: call-b => 1337
 
 ;; CHECK:      [fuzz-exec] calling oob
-;; CHECK-NEXT: [trap callTable overflow]
+;; CHECK-NEXT: [trap out of bounds table access]
 
 ;; CHECK:      [fuzz-exec] calling oob-huge
-;; CHECK-NEXT: [trap callTable overflow]
+;; CHECK-NEXT: [trap out of bounds table access]
 
 ;; CHECK:      [fuzz-exec] calling null
-;; CHECK-NEXT: [trap uninitialized table element]
+;; CHECK-NEXT: [trap null target in call_indirect]
 ;; CHECK-NEXT: [fuzz-exec] comparing call-a
 ;; CHECK-NEXT: [fuzz-exec] comparing call-b
 ;; CHECK-NEXT: [fuzz-exec] comparing null