Clarify what function-parallel passes can do, and fix an asm2wasm bug (#1627)

The problem this fixes is that we made precompute look at globals in #1622, while asm2wasm was creating globals while adding functions and optimizing them - which could race. This was caught by threadSanitizer (with low frequency, so we missed it on the initial landing).

The underlying issue is that function-parallel passes should be able to read global state, just not modify it, and not read other functions' contents (which is why the Call node has a name, not a pointer to a function). This PR clarifies that in the docs, and fixes asm2wasm by not handling function bodies in parallel to creating globals.

3 files changed, 29 insertions, 13 deletions

diff --git a/src/asm2wasm.h b/src/asm2wasm.h
index 4ee09aca5..f6b6da43d 100644
--- a/src/asm2wasm.h
+++ b/src/asm2wasm.h
@@ -973,7 +973,8 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
 
   wasm.table.initial = wasm.table.max = 0;
 
-  // first pass - do almost everything, but function imports and indirect calls
+  // first pass - do all global things, aside from function bodies (second pass)
+  // and function imports and indirect calls (last pass)
 
   for (unsigned i = 1; i < body->size(); i++) {
     Ref curr = body[i];
@@ -1108,17 +1109,6 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
           abort_on("invalid var element", pair);
         }
       }
-    } else if (curr[0] == DEFUN) {
-      // function
-      auto* func = processFunction(curr);
-      if (wasm.getFunctionOrNull(func->name)) {
-        Fatal() << "duplicate function: " << func->name;
-      }
-      if (runOptimizationPasses) {
-        optimizingBuilder->addFunction(func);
-      } else {
-        wasm.addFunction(func);
-      }
     } else if (curr[0] == RETURN) {
       // exports
       Ref object = curr[1];
@@ -1173,6 +1163,23 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
     }
   }
 
+  // second pass: function bodies
+  for (unsigned i = 1; i < body->size(); i++) {
+    Ref curr = body[i];
+    if (curr[0] == DEFUN) {
+      // function
+      auto* func = processFunction(curr);
+      if (wasm.getFunctionOrNull(func->name)) {
+        Fatal() << "duplicate function: " << func->name;
+      }
+      if (runOptimizationPasses) {
+        optimizingBuilder->addFunction(func);
+      } else {
+        wasm.addFunction(func);
+      }
+    }
+  }
+
   if (runOptimizationPasses) {
     optimizingBuilder->finish();
     // if we added any helper functions (like non-trapping i32-div, etc.), then those
@@ -1191,7 +1198,7 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
   }
   wasm.debugInfoFileNames = std::move(preprocessor.debugInfoFileNames);
 
-  // second pass. first, function imports
+  // third pass. first, function imports
 
   std::vector<IString> toErase;
 
diff --git a/src/pass.h b/src/pass.h
index 881fc4e00..30e16761d 100644
--- a/src/pass.h
+++ b/src/pass.h
@@ -211,6 +211,11 @@ public:
   // That means that you can't rely on Walker object properties to persist across
   // your functions, and you can't expect a new object to be created for each
   // function either (which could be very inefficient).
+  //
+  // It is valid for function-parallel passes to read (but not modify) global
+  // module state, like globals or imports. However, reading other functions'
+  // contents is invalid, as function-parallel tests can be run while still
+  // adding functions to the module.
   virtual bool isFunctionParallel() { return false; }
 
   // This method is used to create instances per function for a function-parallel
diff --git a/src/wasm-module-building.h b/src/wasm-module-building.h
index 990ea5191..d03eef7c8 100644
--- a/src/wasm-module-building.h
+++ b/src/wasm-module-building.h
@@ -69,6 +69,10 @@ static std::mutex debug;
 // generally slower than generation; in the optimal case, we never
 // lock beyond the first step, and all further work is lock-free.
 //
+// N.B.: Optimizing functions in parallel with adding functions is possible,
+//       but the rest of the global state of the module should be fixed,
+//       such as globals, imports, etc. Function-parallel optimization passes
+//       may read (but not modify) those fields.
 
 class OptimizingIncrementalModuleBuilder {
   Module* wasm;