refactor wasm.h to remove numericIndex hacks, and move indexing to the parsers

6 files changed, 76 insertions, 69 deletions

diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index 3ce24bdbd..14fc4680d 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -128,6 +128,7 @@ BinaryenFunctionTypeRef BinaryenAddFunctionType(BinaryenModuleRef module, const
   auto* wasm = (Module*)module;
   auto* ret = new FunctionType;
   if (name) ret->name = name;
+  else ret->name = Name::fromInt(wasm->functionTypes.size());
   ret->result = WasmType(result);
   for (BinaryenIndex i = 0; i < numParams; i++) {
     ret->params.push_back(WasmType(paramTypes[i]));
diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index 669a19b89..48639a341 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -55,15 +55,20 @@ struct PatternDatabase {
     input = strdup(
       #include "OptimizeInstructions.wast.processed"
     );
-    SExpressionParser parser(input);
-    Element& root = *parser.root;
-    SExpressionWasmBuilder builder(wasm, *root[0]);
-    // parse module form
-    auto* func = wasm.getFunction("patterns");
-    auto* body = func->body->cast<Block>();
-    for (auto* item : body->list) {
-      auto* pair = item->cast<Block>();
-      patternMap[pair->list[0]->_id].emplace_back(pair->list[0], pair->list[1]);
+    try {
+      SExpressionParser parser(input);
+      Element& root = *parser.root;
+      SExpressionWasmBuilder builder(wasm, *root[0]);
+      // parse module form
+      auto* func = wasm.getFunction("patterns");
+      auto* body = func->body->cast<Block>();
+      for (auto* item : body->list) {
+        auto* pair = item->cast<Block>();
+        patternMap[pair->list[0]->_id].emplace_back(pair->list[0], pair->list[1]);
+      }
+    } catch (ParseException& p) {
+      p.dump(std::cerr);
+      Fatal() << "error in parsing wasm binary";
     }
   }
 
diff --git a/src/wasm-binary.h b/src/wasm-binary.h
index 1bc18e687..85aaf9279 100644
--- a/src/wasm-binary.h
+++ b/src/wasm-binary.h
@@ -652,7 +652,7 @@ public:
       if (debug) std::cerr << "write one at" << o.size() << std::endl;
       size_t sizePos = writeU32LEBPlaceholder();
       size_t start = o.size();
-      Function* function = wasm->getFunction(i);
+      Function* function = wasm->functions[i].get();
       mappedLocals.clear();
       numLocalsByType.clear();
       if (debug) std::cerr << "writing" << function->name << std::endl;
@@ -1495,6 +1495,7 @@ public:
         assert(numResults == 1);
         curr->result = getWasmType();
       }
+      curr->name = Name::fromInt(wasm.functionTypes.size());
       wasm.addFunctionType(curr);
     }
   }
@@ -1526,7 +1527,7 @@ public:
         case Import::Function: {
           auto index = getU32LEB();
           assert(index < wasm.functionTypes.size());
-          curr->functionType = wasm.getFunctionType(index);
+          curr->functionType = wasm.functionTypes[index].get();
           assert(curr->functionType->name.is());
           functionImportIndexes.push_back(curr->name);
           break;
@@ -1551,7 +1552,7 @@ public:
     for (size_t i = 0; i < num; i++) {
       if (debug) std::cerr << "read one" << std::endl;
       auto index = getU32LEB();
-      functionTypes.push_back(wasm.getFunctionType(index));
+      functionTypes.push_back(wasm.functionTypes[index].get());
     }
   }
 
@@ -1659,6 +1660,7 @@ public:
       curr->type = getWasmType();
       curr->init = readExpression();
       curr->mutable_ = true; // TODO
+      curr->name = Name("global$" + std::to_string(wasm.globals.size()));
       wasm.addGlobal(curr);
     }
   }
@@ -1998,7 +2000,7 @@ public:
     if (debug) std::cerr << "zz node: CallIndirect" << std::endl;
     auto arity = getU32LEB();
     WASM_UNUSED(arity);
-    auto* fullType = wasm.getFunctionType(getU32LEB());
+    auto* fullType = wasm.functionTypes.at(getU32LEB()).get();
     curr->fullType = fullType->name;
     auto num = fullType->params.size();
     assert(num == arity);
diff --git a/src/wasm-interpreter.h b/src/wasm-interpreter.h
index 9bd813b84..942ba1ab6 100644
--- a/src/wasm-interpreter.h
+++ b/src/wasm-interpreter.h
@@ -51,14 +51,14 @@ class Flow {
 public:
   Flow() {}
   Flow(Literal value) : value(value) {}
-  Flow(IString breakTo) : breakTo(breakTo) {}
+  Flow(Name breakTo) : breakTo(breakTo) {}
 
   Literal value;
-  IString breakTo; // if non-null, a break is going on
+  Name breakTo; // if non-null, a break is going on
 
   bool breaking() { return breakTo.is(); }
 
-  void clearIf(IString target) {
+  void clearIf(Name target) {
     if (breakTo == target) {
       breakTo.clear();
     }
@@ -589,7 +589,7 @@ private:
   std::vector<Name> functionStack;
 
   // Call a function, starting an invocation.
-  Literal callFunction(IString name, LiteralList& arguments) {
+  Literal callFunction(Name name, LiteralList& arguments) {
     // if the last call ended in a jump up the stack, it might have left stuff for us to clean up here
     callDepth = 0;
     functionStack.clear();
@@ -598,7 +598,7 @@ private:
 
 public:
   // Internal function call. Must be public so that callTable implementations can use it (refactor?)
-  Literal callFunctionInternal(IString name, LiteralList& arguments) {
+  Literal callFunctionInternal(Name name, LiteralList& arguments) {
 
     class FunctionScope {
      public:
@@ -757,7 +757,7 @@ public:
             return Literal(int32_t(ret));
           }
           case HasFeature: {
-            IString id = curr->nameOperand;
+            Name id = curr->nameOperand;
             if (id == WASM) return Literal(1);
             return Literal((int32_t)0);
           }
diff --git a/src/wasm-s-parser.h b/src/wasm-s-parser.h
index f9a43e69b..a914b6598 100644
--- a/src/wasm-s-parser.h
+++ b/src/wasm-s-parser.h
@@ -275,6 +275,8 @@ class SExpressionWasmBuilder {
   Module& wasm;
   MixedArena& allocator;
   std::vector<Name> functionNames;
+  std::vector<Name> functionTypeNames;
+  std::vector<Name> globalNames;
   int functionCounter;
   int globalCounter;
   std::map<Name, WasmType> functionTypes; // we need to know function return types before we parse their contents
@@ -346,8 +348,8 @@ private:
         if (curr.size() > 2) throw ParseException("invalid result arity", curr.line, curr.col);
         functionTypes[name] = stringToWasmType(curr[1]->str());
       } else if (id == TYPE) {
-        Name typeName = curr[1]->str();
-        if (!wasm.checkFunctionType(typeName)) throw ParseException("unknown function", curr.line, curr.col);
+        Name typeName = getFunctionTypeName(*curr[1]);
+        if (!wasm.checkFunctionType(typeName)) throw ParseException("unknown function type", curr.line, curr.col);
         type = wasm.getFunctionType(typeName);
         functionTypes[name] = type->result;
       } else if (id == PARAM && curr.size() > 1) {
@@ -377,6 +379,8 @@ private:
         }
       }
       if (need) {
+        functionType->name = Name::fromInt(wasm.functionTypes.size());
+        functionTypeNames.push_back(functionType->name);
         wasm.addFunctionType(functionType.release());
       }
     }
@@ -441,11 +445,22 @@ private:
     } else {
       // index
       size_t offset = atoi(s.str().c_str());
-      if (offset >= functionNames.size()) throw ParseException("unknown function");
+      if (offset >= functionNames.size()) throw ParseException("unknown function in getFunctionName");
       return functionNames[offset];
     }
   }
 
+  Name getFunctionTypeName(Element& s) {
+    if (s.dollared()) {
+      return s.str();
+    } else {
+      // index
+      size_t offset = atoi(s.str().c_str());
+      if (offset >= functionTypeNames.size()) throw ParseException("unknown function type in getFunctionTypeName");
+      return functionTypeNames[offset];
+    }
+  }
+
   void parseStart(Element& s) {
     wasm.addStart(getFunctionName(*s[1]));
   }
@@ -484,9 +499,18 @@ private:
     size_t i = 1;
     Name name, exportName;
     i = parseFunctionNames(s, name, exportName);
-    if (!name.is()) {
-      // unnamed, use an index
-      name = Name::fromInt(functionCounter);
+    if (!preParseImport) {
+      if (!name.is()) {
+        // unnamed, use an index
+        name = Name::fromInt(functionCounter);
+      }
+      functionCounter++;
+    } else {
+      // just preparsing, functionCounter was incremented by preParseFunctionType
+      if (!name.is()) {
+        // unnamed, use an index
+        name = functionNames[functionCounter - 1];
+      }
     }
     if (exportName.is()) {
       auto ex = make_unique<Export>();
@@ -496,7 +520,6 @@ private:
       if (wasm.checkExport(ex->name)) throw ParseException("duplicate export", s.line, s.col);
       wasm.addExport(ex.release());
     }
-    functionCounter++;
     Expression* body = nullptr;
     localIndex = 0;
     otherIndex = 0;
@@ -555,7 +578,7 @@ private:
         if (curr.size() > 2) throw ParseException("invalid result arity", curr.line, curr.col);
         result = stringToWasmType(curr[1]->str());
       } else if (id == TYPE) {
-        Name name = curr[1]->str();
+        Name name = getFunctionTypeName(*curr[1]);
         type = name;
         if (!wasm.checkFunctionType(name)) throw ParseException("unknown function type");
         FunctionType* type = wasm.getFunctionType(name);
@@ -601,9 +624,6 @@ private:
       assert(preParseImport);
       std::unique_ptr<Import> im = make_unique<Import>();
       im->name = name;
-      if (!im->name.is()) {
-        im->name = name;
-      }
       im->module = importModule;
       im->base = importBase;
       im->kind = Import::Function;
@@ -629,7 +649,6 @@ private:
     if (currFunction->result != result) throw ParseException("bad func declaration", s.line, s.col);
     currFunction->body = body;
     currFunction->type = type;
-
     wasm.addFunction(currFunction.release());
     currLocalTypes.clear();
     labelStack.clear();
@@ -1293,7 +1312,7 @@ private:
   }
 
   Expression* makeCall(Element& s) {
-    auto target = s[1]->str();
+    auto target = getFunctionName(*s[1]);
     auto* import = wasm.checkImport(target);
     if (import && import->kind == Import::Function) {
       auto ret = allocator.alloc<CallImport>();
@@ -1604,8 +1623,10 @@ private:
     if (!im->name.is()) {
       if (im->kind == Import::Function) {
         im->name = Name::fromInt(functionCounter++);
+        functionNames.push_back(im->name);
       } else if (im->kind == Import::Global) {
         im->name = Name::fromInt(globalCounter++);
+        globalNames.push_back(im->name);
       } else if (im->kind == Import::Memory) {
         im->name = Name::fromInt(0);
       } else if (im->kind == Import::Table) {
@@ -1700,6 +1721,7 @@ private:
       global->name = Name::fromInt(globalCounter);
     }
     globalCounter++;
+    globalNames.push_back(global->name);
     bool mutable_ = false;
     WasmType type = none;
     bool exported = false;
@@ -1861,6 +1883,10 @@ private:
         type->result = stringToWasmType(curr[1]->str());
       }
     }
+    if (!type->name.is()) {
+      type->name = Name::fromInt(wasm.functionTypes.size());
+    }
+    functionTypeNames.push_back(type->name);
     wasm.addFunctionType(type.release());
   }
 };
diff --git a/src/wasm.h b/src/wasm.h
index 5c20a3a10..53b4e2938 100644
--- a/src/wasm.h
+++ b/src/wasm.h
@@ -1552,12 +1552,6 @@ private:
 public:
   Module() {};
 
-  FunctionType* getFunctionType(Index i) { assert(i < functionTypes.size()); return functionTypes[i].get(); }
-  Import* getImport(Index i) { assert(i < imports.size()); return imports[i].get(); }
-  Export* getExport(Index i) { assert(i < exports.size()); return exports[i].get(); }
-  Function* getFunction(Index i) { assert(i < functions.size()); return functions[i].get(); }
-  Global* getGlobal(Index i) { assert(i < globals.size()); return globals[i].get(); }
-
   FunctionType* getFunctionType(Name name) { assert(functionTypesMap.count(name)); return functionTypesMap[name]; }
   Import* getImport(Name name) { assert(importsMap.count(name)); return importsMap[name]; }
   Export* getExport(Name name) { assert(exportsMap.count(name)); return exportsMap[name]; }
@@ -1570,56 +1564,35 @@ public:
   Function* checkFunction(Name name) { if (!functionsMap.count(name)) return nullptr; return functionsMap[name]; }
   Global* checkGlobal(Name name) { if (!globalsMap.count(name)) return nullptr; return globalsMap[name]; }
 
-  FunctionType* checkFunctionType(Index i) { if (i >= functionTypes.size()) return nullptr; return functionTypes[i].get(); }
-  Import* checkImport(Index i) { if (i >= imports.size()) return nullptr; return imports[i].get(); }
-  Export* checkExport(Index i) { if (i >= exports.size()) return nullptr; return exports[i].get(); }
-  Function* checkFunction(Index i) { if (i >= functions.size()) return nullptr; return functions[i].get(); }
-  Global* checkGlobal(Index i) { if (i >= globals.size()) return nullptr; return globals[i].get(); }
-
   void addFunctionType(FunctionType* curr) {
-    Name numericName = Name::fromInt(functionTypes.size()); // TODO: remove all these, assert on names already existing, do numeric stuff in wasm-s-parser etc.
-    if (curr->name.isNull()) {
-      curr->name = numericName;
-    }
+    assert(curr->name.is());
     functionTypes.push_back(std::unique_ptr<FunctionType>(curr));
+    assert(functionTypesMap.find(curr->name) == functionTypesMap.end());
     functionTypesMap[curr->name] = curr;
-    functionTypesMap[numericName] = curr;
   }
   void addImport(Import* curr) {
-    Name numericName = Name::fromInt(imports.size());
-    if (curr->name.isNull()) {
-      curr->name = numericName;
-    }
+    assert(curr->name.is());
     imports.push_back(std::unique_ptr<Import>(curr));
+    assert(importsMap.find(curr->name) == importsMap.end());
     importsMap[curr->name] = curr;
-    importsMap[numericName] = curr;
   }
   void addExport(Export* curr) {
-    Name numericName = Name::fromInt(exports.size());
-    if (curr->name.isNull()) {
-      curr->name = numericName;
-    }
+    assert(curr->name.is());
     exports.push_back(std::unique_ptr<Export>(curr));
+    assert(exportsMap.find(curr->name) == exportsMap.end());
     exportsMap[curr->name] = curr;
-    exportsMap[numericName] = curr;
   }
   void addFunction(Function* curr) {
-    Name numericName = Name::fromInt(functions.size());
-    if (curr->name.isNull()) {
-      curr->name = numericName;
-    }
+    assert(curr->name.is());
     functions.push_back(std::unique_ptr<Function>(curr));
+    assert(functionsMap.find(curr->name) == functionsMap.end());
     functionsMap[curr->name] = curr;
-    functionsMap[numericName] = curr;
   }
   void addGlobal(Global* curr) {
-    Name numericName = Name::fromInt(globals.size());
-    if (curr->name.isNull()) {
-      curr->name = numericName;
-    }
+    assert(curr->name.is());
     globals.push_back(std::unique_ptr<Global>(curr));
+    assert(globalsMap.find(curr->name) == globalsMap.end());
     globalsMap[curr->name] = curr;
-    globalsMap[numericName] = curr;
   }
 
   void addStart(const Name &s) {