Proper error handling in add* and get* methods (#1570)

See #1479 (comment)

Also a one-line readme update, remove an obsolete compiler (mir2wasm) and add a new one (asterius).

Also improve warning and error reporting in binaryen.js - show a stack trace when relevant (instead of node.js process.exit), and avoid atexit warning spam in debug builds.

9 files changed, 116 insertions, 38 deletions

diff --git a/README.md b/README.md
index 6d4bc7aff..d16e1eafe 100644
--- a/README.md
+++ b/README.md
@@ -13,7 +13,7 @@ Compilers built using Binaryen include
  * [`asm2wasm`](https://github.com/WebAssembly/binaryen/blob/master/src/asm2wasm.h) which compiles asm.js to WebAssembly
  * [`AssemblyScript`](https://github.com/AssemblyScript/assemblyscript) which compiles TypeScript to Binaryen IR
  * [`wasm2asm`](https://github.com/WebAssembly/binaryen/blob/master/src/wasm2asm.h) which compiles WebAssembly to asm.js
- * [`mir2wasm`](https://github.com/brson/mir2wasm/) which compiles Rust MIR
+ * [`Asterius`](https://github.com/tweag/asterius) which compiles Haskell to WebAssembly
 
 Binaryen also provides a set of **toolchain utilities** that can
 
diff --git a/auto_update_tests.py b/auto_update_tests.py
index 3598420ce..181eb5696 100755
--- a/auto_update_tests.py
+++ b/auto_update_tests.py
@@ -313,7 +313,11 @@ def update_binaryen_js_tests():
     f.close()
     if MOZJS or node_has_wasm or 'WebAssembly.' not in test_src:
       cmd = [MOZJS or NODEJS, 'a.js']
-      out = run_command(cmd, stderr=subprocess.STDOUT)
+      if 'fatal' not in s:
+        out = run_command(cmd, stderr=subprocess.STDOUT)
+      else:
+        # expect an error - the specific error code will depend on the vm
+        out = run_command(cmd, stderr=subprocess.STDOUT, expected_status=None)
       with open(os.path.join('test', 'binaryen.js', s + '.txt'), 'w') as o:
         o.write(out)
     else:
diff --git a/check.py b/check.py
index cf99b1bf3..a905b098c 100755
--- a/check.py
+++ b/check.py
@@ -436,7 +436,11 @@ def run_binaryen_js_tests():
 
     def test(engine):
       cmd = [engine, 'a.js']
-      out = run_command(cmd, stderr=subprocess.STDOUT)
+      if 'fatal' not in s:
+        out = run_command(cmd, stderr=subprocess.STDOUT)
+      else:
+        # expect an error - the specific error code will depend on the vm
+        out = run_command(cmd, stderr=subprocess.STDOUT, expected_status=None)
       expected = open(os.path.join(options.binaryen_test, 'binaryen.js', s + '.txt')).read()
       if expected not in out:
         fail(out, expected)
@@ -510,7 +514,7 @@ def run_vanilla_tests():
       del os.environ['EMCC_WASM_BACKEND']
 
 
-def run_gcc_torture_tests():
+def run_gcc_tests():
   print '\n[ checking native gcc testcases...]\n'
   if not NATIVECC or not NATIVEXX:
     fail_with_error('Native compiler (e.g. gcc/g++) was not found in PATH!')
@@ -648,7 +652,7 @@ def main():
     run_vanilla_tests()
   print '\n[ checking example testcases... ]\n'
   if options.run_gcc_tests:
-    run_gcc_torture_tests()
+    run_gcc_tests()
   if EMCC:
     run_emscripten_tests()
 
diff --git a/scripts/test/support.py b/scripts/test/support.py
index f23e1c4de..d04a85c59 100644
--- a/scripts/test/support.py
+++ b/scripts/test/support.py
@@ -156,7 +156,7 @@ def run_command(cmd, expected_status=0, stderr=None,
   proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=stderr, universal_newlines=True)
   out, err = proc.communicate()
   code = proc.returncode
-  if code != expected_status:
+  if expected_status is not None and code != expected_status:
     raise Exception(('run_command failed (%s)' % code, out + str(err or '')))
   err_correct = expected_err is None or \
       (expected_err in err if err_contains else expected_err == err)
diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index 4dd306bd3..33281ff82 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -2601,4 +2601,12 @@ BinaryenFunctionTypeRef BinaryenGetFunctionTypeBySignature(BinaryenModuleRef mod
   return NULL;
 }
 
+#ifdef __EMSCRIPTEN__
+// Override atexit - we don't need any global ctors to actually run, and
+// otherwise we get clutter in the output in debug builds
+int atexit(void (*function)(void)) {
+  return 0;
+}
+#endif
+
 } // extern "C"
diff --git a/src/js/binaryen.js-post.js b/src/js/binaryen.js-post.js
index 6e4833986..b61b8c3e2 100644
--- a/src/js/binaryen.js-post.js
+++ b/src/js/binaryen.js-post.js
@@ -1609,3 +1609,12 @@ Module['setDebugInfo'] = function(on) {
 Module['setAPITracing'] = function(on) {
   return Module['_BinaryenSetAPITracing'](on);
 };
+
+// Additional customizations
+
+Module['exit'] = function(status) {
+  // Instead of exiting silently on errors, always show an error with
+  // a stack trace, for debuggability.
+  if (status != 0) throw new Error('exiting due to error: ' + status);
+};
+
diff --git a/src/wasm/wasm.cpp b/src/wasm/wasm.cpp
index 49b1f10e4..d88ab0fcd 100644
--- a/src/wasm/wasm.cpp
+++ b/src/wasm/wasm.cpp
@@ -619,8 +619,11 @@ Name Function::getLocalNameOrGeneric(Index index) {
 }
 
 Index Function::getLocalIndex(Name name) {
-  assert(localIndices.count(name) > 0);
-  return localIndices[name];
+  auto iter = localIndices.find(name);
+  if (iter == localIndices.end()) {
+    Fatal() << "Function::getLocalIndex: " << name << " does not exist";
+  }
+  return iter->second;
 }
 
 Index Function::getVarIndexBase() {
@@ -638,92 +641,137 @@ Type Function::getLocalType(Index index) {
 }
 
 FunctionType* Module::getFunctionType(Name name) {
-  assert(functionTypesMap.count(name));
-  return functionTypesMap[name];
+  auto iter = functionTypesMap.find(name);
+  if (iter == functionTypesMap.end()) {
+    Fatal() << "Module::getFunctionType: " << name << " does not exist";
+  }
+  return iter->second;
 }
 
 Import* Module::getImport(Name name) {
-  assert(importsMap.count(name));
-  return importsMap[name];
+  auto iter = importsMap.find(name);
+  if (iter == importsMap.end()) {
+    Fatal() << "Module::getImport: " << name << " does not exist";
+  }
+  return iter->second;
 }
 
 Export* Module::getExport(Name name) {
-  assert(exportsMap.count(name));
-  return exportsMap[name];
+  auto iter = exportsMap.find(name);
+  if (iter == exportsMap.end()) {
+    Fatal() << "Module::getExport: " << name << " does not exist";
+  }
+  return iter->second;
 }
 
 Function* Module::getFunction(Name name) {
-  assert(functionsMap.count(name));
-  return functionsMap[name];
+  auto iter = functionsMap.find(name);
+  if (iter == functionsMap.end()) {
+    Fatal() << "Module::getFunction: " << name << " does not exist";
+  }
+  return iter->second;
 }
 
 Global* Module::getGlobal(Name name) {
-  assert(globalsMap.count(name));
-  return globalsMap[name];
+  auto iter = globalsMap.find(name);
+  if (iter == globalsMap.end()) {
+    Fatal() << "Module::getGlobal: " << name << " does not exist";
+  }
+  return iter->second;
 }
 
 FunctionType* Module::getFunctionTypeOrNull(Name name) {
-  if (!functionTypesMap.count(name))
+  auto iter = functionTypesMap.find(name);
+  if (iter == functionTypesMap.end()) {
     return nullptr;
-  return functionTypesMap[name];
+  }
+  return iter->second;
 }
 
 Import* Module::getImportOrNull(Name name) {
-  if (!importsMap.count(name))
+  auto iter = importsMap.find(name);
+  if (iter == importsMap.end()) {
     return nullptr;
-  return importsMap[name];
+  }
+  return iter->second;
 }
 
 Export* Module::getExportOrNull(Name name) {
-  if (!exportsMap.count(name))
+  auto iter = exportsMap.find(name);
+  if (iter == exportsMap.end()) {
     return nullptr;
-  return exportsMap[name];
+  }
+  return iter->second;
 }
 
 Function* Module::getFunctionOrNull(Name name) {
-  if (!functionsMap.count(name))
+  auto iter = functionsMap.find(name);
+  if (iter == functionsMap.end()) {
     return nullptr;
-  return functionsMap[name];
+  }
+  return iter->second;
 }
 
 Global* Module::getGlobalOrNull(Name name) {
-  if (!globalsMap.count(name))
+  auto iter = globalsMap.find(name);
+  if (iter == globalsMap.end()) {
     return nullptr;
-  return globalsMap[name];
+  }
+  return iter->second;
 }
 
 void Module::addFunctionType(FunctionType* curr) {
-  assert(curr->name.is());
+  if (!curr->name.is()) {
+    Fatal() << "Module::addFunctionType: empty name";
+  }
+  if (getFunctionTypeOrNull(curr->name)) {
+    Fatal() << "Module::addFunctionType: " << curr->name << " already exists";
+  }
   functionTypes.push_back(std::unique_ptr<FunctionType>(curr));
-  assert(functionTypesMap.find(curr->name) == functionTypesMap.end());
   functionTypesMap[curr->name] = curr;
 }
 
 void Module::addImport(Import* curr) {
-  assert(curr->name.is());
+  if (!curr->name.is()) {
+    Fatal() << "Module::addImport: empty name";
+  }
+  if (getImportOrNull(curr->name)) {
+    Fatal() << "Module::addImport: " << curr->name << " already exists";
+  }
   imports.push_back(std::unique_ptr<Import>(curr));
-  assert(importsMap.find(curr->name) == importsMap.end());
   importsMap[curr->name] = curr;
 }
 
 void Module::addExport(Export* curr) {
-  assert(curr->name.is());
+  if (!curr->name.is()) {
+    Fatal() << "Module::addExport: empty name";
+  }
+  if (getExportOrNull(curr->name)) {
+    Fatal() << "Module::addExport: " << curr->name << " already exists";
+  }
   exports.push_back(std::unique_ptr<Export>(curr));
-  assert(exportsMap.find(curr->name) == exportsMap.end());
   exportsMap[curr->name] = curr;
 }
 
 void Module::addFunction(Function* curr) {
-  assert(curr->name.is());
+  if (!curr->name.is()) {
+    Fatal() << "Module::addFunction: empty name";
+  }
+  if (getFunctionOrNull(curr->name)) {
+    Fatal() << "Module::addFunction: " << curr->name << " already exists";
+  }
   functions.push_back(std::unique_ptr<Function>(curr));
-  assert(functionsMap.find(curr->name) == functionsMap.end());
   functionsMap[curr->name] = curr;
 }
 
 void Module::addGlobal(Global* curr) {
-  assert(curr->name.is());
+  if (!curr->name.is()) {
+    Fatal() << "Module::addGlobal: empty name";
+  }
+  if (getGlobalOrNull(curr->name)) {
+    Fatal() << "Module::addGlobal: " << curr->name << " already exists";
+  }
   globals.push_back(std::unique_ptr<Global>(curr));
-  assert(globalsMap.find(curr->name) == globalsMap.end());
   globalsMap[curr->name] = curr;
 }
 
diff --git a/test/binaryen.js/fatal.js b/test/binaryen.js/fatal.js
new file mode 100644
index 000000000..5e8071daa
--- /dev/null
+++ b/test/binaryen.js/fatal.js
@@ -0,0 +1,4 @@
+var wasm = new Binaryen.Module()
+wasm.addFunctionType("vI", Binaryen.i32, []);
+// It will cause a fatal error to try to add the same name a second time
+wasm.addFunctionType("vI", Binaryen.i32, []);
diff --git a/test/binaryen.js/fatal.js.txt b/test/binaryen.js/fatal.js.txt
new file mode 100644
index 000000000..3c1e7a47c
--- /dev/null
+++ b/test/binaryen.js/fatal.js.txt
@@ -0,0 +1 @@
+Fatal: Module::addFunctionType: $vI already exists