Preserve debug info through the optimizer (#981)

* add debugInfo option to passes, and use it to keep debug info alive through optimizations when we need it

* add fib testcase for debug info

* when preserving debug info, do not move code around call-imports, so debug info intrinsics remain stationary

* improve wasm-module-building handling of the single-threaded case: don't create workers, which is more efficient and also nicer for debugging

* process debug info in a more precise way, reordering it from being after the node (as it was a comment in JS) to before the node

* remove unreachable hack for debug info, which is no longer needed since we reorder them, and make sure to finalize blocks in which we reorder

7 files changed, 138 insertions, 68 deletions

diff --git a/src/asm2wasm.h b/src/asm2wasm.h
index ed8c75e56..f78b9df55 100644
--- a/src/asm2wasm.h
+++ b/src/asm2wasm.h
@@ -303,6 +303,44 @@ struct Asm2WasmPreProcessor {
   }
 };
 
+static CallImport* checkDebugInfo(Expression* curr) {
+  if (auto* call = curr->dynCast<CallImport>()) {
+    if (call->target == EMSCRIPTEN_DEBUGINFO) {
+      return call;
+    }
+  }
+  return nullptr;
+}
+
+// Debug info appears in the ast as calls to the debug intrinsic. These are usually
+// after the relevant node. We adjust them to a position that is not dce-able, so that
+// they are not trivially removed when optimizing.
+struct AdjustDebugInfo : public WalkerPass<PostWalker<AdjustDebugInfo, Visitor<AdjustDebugInfo>>> {
+  bool isFunctionParallel() override { return true; }
+
+  Pass* create() override { return new AdjustDebugInfo(); }
+
+  AdjustDebugInfo() {
+    name = "adjust-debug-info";
+  }
+
+  void visitBlock(Block* curr) {
+    // look for a debug info call that is unreachable
+    if (curr->list.size() == 0) return;
+    auto* back = curr->list.back();
+    for (Index i = 1; i < curr->list.size(); i++) {
+      if (checkDebugInfo(curr->list[i]) && !checkDebugInfo(curr->list[i - 1])) {
+        // swap them
+        std::swap(curr->list[i - 1], curr->list[i]);
+      }
+    }
+    if (curr->list.back() != back) {
+      // we changed the last element, update the type
+      curr->finalize();
+    }
+  }
+};
+
 //
 // Asm2WasmBuilder - converts an asm.js module into WebAssembly
 //
@@ -841,16 +879,6 @@ private:
   }
 
   Function* processFunction(Ref ast);
-
-public:
-  CallImport* checkDebugInfo(Expression* curr) {
-    if (auto* call = curr->dynCast<CallImport>()) {
-      if (call->target == EMSCRIPTEN_DEBUGINFO) {
-        return call;
-      }
-    }
-    return nullptr;
-  }
 };
 
 void Asm2WasmBuilder::processAsm(Ref ast) {
@@ -976,6 +1004,10 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
       }
       // run autodrop first, before optimizations
       passRunner.add<AutoDrop>();
+      if (preprocessor.debugInfo) {
+        // fix up debug info to better survive optimization
+        passRunner.add<AdjustDebugInfo>();
+      }
       // optimize relooper label variable usage at the wasm level, where it is easy
       passRunner.add("relooper-jump-threading");
     }, debug, false /* do not validate globally yet */);
@@ -1302,39 +1334,55 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
   };
 
   // apply debug info, reducing intrinsic calls into annotations on the ast nodes
-  struct ApplyDebugInfo : public WalkerPass<PostWalker<ApplyDebugInfo, UnifiedExpressionVisitor<ApplyDebugInfo>>> {
+  struct ApplyDebugInfo : public WalkerPass<ExpressionStackWalker<ApplyDebugInfo, UnifiedExpressionVisitor<ApplyDebugInfo>>> {
     bool isFunctionParallel() override { return true; }
 
-    Pass* create() override { return new ApplyDebugInfo(parent); }
+    Pass* create() override { return new ApplyDebugInfo(); }
 
-    Asm2WasmBuilder* parent;
-
-    ApplyDebugInfo(Asm2WasmBuilder* parent) : parent(parent) {
+    ApplyDebugInfo() {
       name = "apply-debug-info";
     }
 
-    Expression* lastExpression = nullptr;
+    CallImport* lastDebugInfo = nullptr;
 
     void visitExpression(Expression* curr) {
-      if (auto* call = parent->checkDebugInfo(curr)) {
-        // this is a debuginfo node. turn it into an annotation on the last stack
-        auto* last = lastExpression;
-        lastExpression = nullptr;
-        auto& debugLocations = getFunction()->debugLocations;
-        if (last) {
-          uint32_t fileIndex = call->operands[0]->cast<Const>()->value.geti32();
+      if (auto* call = checkDebugInfo(curr)) {
+        lastDebugInfo = call;
+        replaceCurrent(getModule()->allocator.alloc<Nop>());
+      } else {
+        if (lastDebugInfo) {
+          auto& debugLocations = getFunction()->debugLocations;
+          uint32_t fileIndex = lastDebugInfo->operands[0]->cast<Const>()->value.geti32();
           assert(getModule()->debugInfoFileNames.size() > fileIndex);
-          uint32_t lineNumber = call->operands[1]->cast<Const>()->value.geti32();
-          debugLocations[last] = {fileIndex, lineNumber};
+          uint32_t lineNumber = lastDebugInfo->operands[1]->cast<Const>()->value.geti32();
+          // look up the stack, apply to the root expression
+          Index i = expressionStack.size() - 1;
+          while (1) {
+            auto* exp = expressionStack[i];
+            bool parentIsStructure = i > 0 && (expressionStack[i - 1]->is<Block>() ||
+                                               expressionStack[i - 1]->is<Loop>() ||
+                                               expressionStack[i - 1]->is<If>());
+            if (i == 0 || parentIsStructure || exp->type == none || exp->type == unreachable) {
+              if (debugLocations.count(exp) > 0) {
+                // already present, so look back up
+                i++;
+                while (i < expressionStack.size()) {
+                  exp = expressionStack[i];
+                  if (debugLocations.count(exp) == 0) {
+                    debugLocations[exp] = { fileIndex, lineNumber };
+                    break;
+                  }
+                  i++;
+                }
+              } else {
+                debugLocations[exp] = { fileIndex, lineNumber };
+              }
+              break;
+            }
+            i--;
+          }
+          lastDebugInfo = nullptr;
         }
-        // eliminate the debug info call
-        ExpressionManipulator::nop(curr);
-        return;
-      }
-      // ignore const nodes, as they may be the children of the debug info calls, and they
-      // don't really need debug info anyhow
-      if (!curr->is<Const>()) {
-        lastExpression = curr;
       }
     }
   };
@@ -1354,9 +1402,15 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
     passRunner.add("remove-unused-brs");
     passRunner.add("optimize-instructions");
     passRunner.add("post-emscripten");
+  } else {
+    if (preprocessor.debugInfo) {
+      // we would have run this before if optimizing, do it now otherwise. must
+      // precede ApplyDebugInfo
+      passRunner.add<AdjustDebugInfo>();
+    }
   }
   if (preprocessor.debugInfo) {
-    passRunner.add<ApplyDebugInfo>(this);
+    passRunner.add<ApplyDebugInfo>();
     passRunner.add("vacuum"); // FIXME maybe just remove the nops that were debuginfo nodes, if not optimizing?
   }
   passRunner.run();
@@ -2558,27 +2612,6 @@ Function* Asm2WasmBuilder::processFunction(Ref ast) {
   };
   // body
   function->body = processStatements(body, start);
-  // debug info cleanup: we add debug info calls after each instruction; as
-  // a result,
-  //   return 0; //@line file.cpp
-  // will have code after the return. if the function body is a block,
-  // it will be forced to the return type of the function, and then
-  // the unreachable type of the return makes things work, which we break
-  // if we add a none debug intrinsic call afterwards. so we need to fix
-  // that up.
-  if (preprocessor.debugInfo) {
-    if (function->result != none) {
-      if (auto* block = function->body->dynCast<Block>()) {
-        if (block->list.size() > 0) {
-          if (checkDebugInfo(block->list.back())) {
-            // add an unreachable. both the debug info and it could be dce'd,
-            // but it makes us validate properly.
-            block->list.push_back(builder.makeUnreachable());
-          }
-        }
-      }
-    }
-  }
   // cleanups/checks
   assert(breakStack.size() == 0 && continueStack.size() == 0);
   assert(parentLabel.isNull());
diff --git a/src/ast_utils.h b/src/ast_utils.h
index 00b755bf7..17627d959 100644
--- a/src/ast_utils.h
+++ b/src/ast_utils.h
@@ -73,10 +73,12 @@ struct BreakSeeker : public PostWalker<BreakSeeker> {
 struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
   EffectAnalyzer(PassOptions& passOptions, Expression *ast = nullptr) {
     ignoreImplicitTraps = passOptions.ignoreImplicitTraps;
+    debugInfo = passOptions.debugInfo;
     if (ast) analyze(ast);
   }
 
   bool ignoreImplicitTraps;
+  bool debugInfo;
 
   void analyze(Expression *ast) {
     breakNames.clear();
@@ -187,7 +189,14 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
   }
 
   void visitCall(Call *curr) { calls = true; }
-  void visitCallImport(CallImport *curr) { calls = true; }
+  void visitCallImport(CallImport *curr) {
+    calls = true;
+    if (debugInfo) {
+      // debugInfo call imports must be preserved very strongly, do not
+      // move code around them
+      branches = true; // !
+    }
+  }
   void visitCallIndirect(CallIndirect *curr) { calls = true; }
   void visitGetLocal(GetLocal *curr) {
     localsRead.insert(curr->index);
diff --git a/src/pass.h b/src/pass.h
index faf674c49..3f0c2c29d 100644
--- a/src/pass.h
+++ b/src/pass.h
@@ -61,6 +61,7 @@ struct PassOptions {
   int optimizeLevel = 0; // 0, 1, 2 correspond to -O0, -O1, -O2, etc.
   int shrinkLevel = 0;   // 0, 1, 2 correspond to -O0, -Os, -Oz
   bool ignoreImplicitTraps = false; // optimize assuming things like div by 0, bad load/store, will not trap
+  bool debugInfo = false; // whether to try to preserve debug info through, which are special calls
 };
 
 //
diff --git a/src/passes/CoalesceLocals.cpp b/src/passes/CoalesceLocals.cpp
index ce9b2d291..ed075c57f 100644
--- a/src/passes/CoalesceLocals.cpp
+++ b/src/passes/CoalesceLocals.cpp
@@ -657,6 +657,7 @@ void CoalesceLocals::applyIndices(std::vector<Index>& indices, Expression* root)
           }
           continue;
         }
+        // remove ineffective actions
         if (!action.effective) {
           *action.origin = set->value; // value may have no side effects, further optimizations can eliminate it
           if (!set->isTee()) {
diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp
index 577a460cb..8da42f9cd 100644
--- a/src/passes/pass.cpp
+++ b/src/passes/pass.cpp
@@ -17,6 +17,7 @@
 #include <chrono>
 #include <sstream>
 
+#include <support/colors.h>
 #include <passes/passes.h>
 #include <pass.h>
 #include <wasm-validator.h>
@@ -111,7 +112,9 @@ void PassRunner::addDefaultOptimizationPasses() {
 }
 
 void PassRunner::addDefaultFunctionOptimizationPasses() {
-  add("dce");
+  if (!options.debugInfo) { // debug info must be preserved, do not dce it
+    add("dce");
+  }
   add("remove-unused-brs");
   add("remove-unused-names");
   add("optimize-instructions");
@@ -147,6 +150,17 @@ void PassRunner::addDefaultGlobalOptimizationPasses() {
   add("memory-packing");
 }
 
+static void dumpWast(Name name, Module* wasm) {
+  // write out the wast
+  Colors::disable();
+  static int counter = 0;
+  std::stringstream text;
+  WasmPrinter::printModule(wasm, text);
+  FILE* f = fopen((std::string("byn-") + std::to_string(counter++) + "-" + name.str + ".wast").c_str(), "w");
+  fputs(text.str().c_str(), f);
+  fclose(f);
+}
+
 void PassRunner::run() {
   // BINARYEN_PASS_DEBUG is a convenient commandline way to log out the toplevel passes, their times,
   //                     and validate between each pass.
@@ -161,6 +175,9 @@ void PassRunner::run() {
     for (auto pass : passes) {
       padding = std::max(padding, pass->name.size());
     }
+    if (passDebug >= 3) {
+      dumpWast("before", wasm);
+    }
     for (auto* pass : passes) {
       // ignoring the time, save a printout of the module before, in case this pass breaks it, so we can print the before and after
       std::stringstream moduleBefore;
@@ -195,6 +212,9 @@ void PassRunner::run() {
         }
         abort();
       }
+      if (passDebug >= 3) {
+        dumpWast(pass->name, wasm);
+      }
     }
     std::cerr << "[PassRunner] passes took " << totalTime.count() << " seconds." << std::endl;
     // validate
diff --git a/src/tools/asm2wasm.cpp b/src/tools/asm2wasm.cpp
index 0679ad3f5..4ad5a5814 100644
--- a/src/tools/asm2wasm.cpp
+++ b/src/tools/asm2wasm.cpp
@@ -35,7 +35,6 @@ int main(int argc, const char *argv[]) {
   bool runOptimizationPasses = false;
   Asm2WasmBuilder::TrapMode trapMode = Asm2WasmBuilder::TrapMode::JS;
   bool wasmOnly = false;
-  bool debugInfo = false;
   std::string symbolMap;
   bool emitBinary = true;
 
@@ -98,7 +97,7 @@ int main(int argc, const char *argv[]) {
            })
       .add("--debuginfo", "-g", "Emit names section and debug info (for debug info you must emit text, -S, for this to work)",
            Options::Arguments::Zero,
-           [&](Options *o, const std::string &arguments) { debugInfo = true; })
+           [&](Options *o, const std::string &arguments) { passOptions.debugInfo = true; })
       .add("--symbolmap", "-s", "Emit a symbol map (indexes => names)",
            Options::Arguments::One,
            [&](Options *o, const std::string &argument) { symbolMap = argument; })
@@ -128,7 +127,7 @@ int main(int argc, const char *argv[]) {
 
   Asm2WasmPreProcessor pre;
   // wasm binaries can contain a names section, but not full debug info
-  pre.debugInfo = debugInfo && !emitBinary;
+  pre.debugInfo = passOptions.debugInfo && !emitBinary;
   auto input(
       read_file<std::vector<char>>(options.extra["infile"], Flags::Text, options.debug ? Flags::Debug : Flags::Release));
   char *start = pre.process(input.data());
@@ -188,7 +187,7 @@ int main(int argc, const char *argv[]) {
   if (options.debug) std::cerr << "printing..." << std::endl;
   ModuleWriter writer;
   writer.setDebug(options.debug);
-  writer.setDebugInfo(debugInfo);
+  writer.setDebugInfo(passOptions.debugInfo);
   writer.setSymbolMap(symbolMap);
   writer.setBinary(emitBinary);
   writer.write(wasm, options.extra["output"]);
diff --git a/src/wasm-module-building.h b/src/wasm-module-building.h
index 4123a59c0..88efb7748 100644
--- a/src/wasm-module-building.h
+++ b/src/wasm-module-building.h
@@ -94,8 +94,8 @@ public:
       : wasm(wasm), numFunctions(numFunctions), passOptions(passOptions), addPrePasses(addPrePasses), endMarker(nullptr), list(nullptr), nextFunction(0),
         numWorkers(0), liveWorkers(0), activeWorkers(0), availableFuncs(0), finishedFuncs(0),
         finishing(false), debug(debug), validateGlobally(validateGlobally) {
-    if (numFunctions == 0 || debug) {
-      // if no functions to be optimized, or debug non-parallel mode, don't create any threads.
+    if (!useWorkers()) {
+      // if we shouldn't use threads, don't
       return;
     }
 
@@ -117,6 +117,7 @@ public:
     DEBUG_THREAD("creating workers");
     numWorkers = ThreadPool::getNumCores();
     assert(numWorkers >= 1);
+    // worth it to use threads
     liveWorkers.store(0);
     activeWorkers.store(0);
     for (uint32_t i = 0; i < numWorkers; i++) { // TODO: one less, and add it at the very end, to not compete with main thread?
@@ -134,10 +135,14 @@ public:
     delete endMarker;
   }
 
+  bool useWorkers() {
+    return numFunctions > 0 && !debug && ThreadPool::getNumCores() > 1;
+  }
+
   // Add a function to the module, and to be optimized
   void addFunction(Function* func) {
     wasm->addFunction(func);
-    if (debug) return; // we optimize at the end if debugging
+    if (!useWorkers()) return; // we optimize at the end in that case
     queueFunction(func);
     // wake workers if needed
     auto wake = availableFuncs.load();
@@ -149,12 +154,14 @@ public:
   // All functions have been added, block until all are optimized, and then do
   // global optimizations. When this returns, the module is ready and optimized.
   void finish() {
-    if (debug) {
-      // in debug mode, optimize each function now that we are done adding functions,
+    if (!useWorkers()) {
+      // optimize each function now that we are done adding functions,
       // then optimize globally
       PassRunner passRunner(wasm, passOptions);
-      passRunner.setDebug(true);
-      passRunner.setValidateGlobally(validateGlobally);
+      if (debug) {
+        passRunner.setDebug(true);
+        passRunner.setValidateGlobally(validateGlobally);
+      }
       addPrePasses(passRunner);
       passRunner.addDefaultFunctionOptimizationPasses();
       passRunner.addDefaultGlobalOptimizationPasses();