Warn when running a pass not compatible with DWARF (#3506)

Previously the addDefault* methods would avoid adding opt passes that we
know are incompatible with DWARF. However, that didn't handle the case of
passes that are added in other ways. For example, when running Asyncify,
emcc will run --flatten before, and that pass is not compatible with DWARF.
This PR lets us warn on that by annotating the passes themselves. Then we
use those annotation to either not run a pass at all (matching the previous
behavior) or to show a warning when necessary.

Fixes emscripten-core/emscripten#13288 . That is, concretely
after this PR running asyncify + DWARF will show a warning to the user.

12 files changed, 135 insertions, 93 deletions

diff --git a/src/pass.h b/src/pass.h
index 3cfa585a7..7a830acb5 100644
--- a/src/pass.h
+++ b/src/pass.h
@@ -183,11 +183,7 @@ struct PassRunner {
 
   // Add a pass using its name.
   void add(std::string passName) {
-    auto pass = PassRegistry::get()->createPass(passName);
-    if (!pass) {
-      Fatal() << "Could not find pass: " << passName << "\n";
-    }
-    doAdd(std::move(pass));
+    doAdd(PassRegistry::get()->createPass(passName));
   }
 
   // Add a pass given an instance.
@@ -195,6 +191,16 @@ struct PassRunner {
     doAdd(std::move(pass));
   }
 
+  // Adds the pass if there are no DWARF-related issues. There is an issue if
+  // there is DWARF and if the pass does not support DWARF (as defined by the
+  // pass returning true from invalidatesDWARF); otherwise, if there is no
+  // DWARF, or the pass supports it, the pass is added.
+  // In contrast to add(), add() will always add the pass, and it will print a
+  // warning if there is an issue with DWARF. This method is useful for a pass
+  // that is optional, to avoid adding it and therefore avoid getting the
+  // warning.
+  void addIfNoDWARFIssues(std::string passName);
+
   // Adds the default set of optimization passes; this is
   // what -O does.
   void addDefaultOptimizationPasses();
@@ -312,6 +318,10 @@ public:
   // out any Stack IR - it would need to be regenerated and optimized.
   virtual bool modifiesBinaryenIR() { return true; }
 
+  // Some passes modify the wasm in a way that we cannot update DWARF properly
+  // for. This is used to issue a proper warning about that.
+  virtual bool invalidatesDWARF() { return false; }
+
   std::string name;
 
 protected:
diff --git a/src/passes/CoalesceLocals.cpp b/src/passes/CoalesceLocals.cpp
index e5416971b..ad64b066c 100644
--- a/src/passes/CoalesceLocals.cpp
+++ b/src/passes/CoalesceLocals.cpp
@@ -46,6 +46,10 @@ struct CoalesceLocals
   : public WalkerPass<LivenessWalker<CoalesceLocals, Visitor<CoalesceLocals>>> {
   bool isFunctionParallel() override { return true; }
 
+  // This pass merges locals, mapping the originals to new ones.
+  // FIXME DWARF updating does not handle local changes yet.
+  bool invalidatesDWARF() override { return true; }
+
   Pass* create() override { return new CoalesceLocals; }
 
   // main entry point
diff --git a/src/passes/DeadArgumentElimination.cpp b/src/passes/DeadArgumentElimination.cpp
index 34637cf5a..f483a2295 100644
--- a/src/passes/DeadArgumentElimination.cpp
+++ b/src/passes/DeadArgumentElimination.cpp
@@ -254,6 +254,10 @@ struct DAEScanner
 };
 
 struct DAE : public Pass {
+  // This pass changes locals and parameters.
+  // FIXME DWARF updating does not handle local changes yet.
+  bool invalidatesDWARF() override { return true; }
+
   bool optimize = false;
 
   void run(PassRunner* runner, Module* module) override {
diff --git a/src/passes/DuplicateFunctionElimination.cpp b/src/passes/DuplicateFunctionElimination.cpp
index 8977e035f..ce17a0a07 100644
--- a/src/passes/DuplicateFunctionElimination.cpp
+++ b/src/passes/DuplicateFunctionElimination.cpp
@@ -31,6 +31,9 @@
 namespace wasm {
 
 struct DuplicateFunctionElimination : public Pass {
+  // FIXME Merge DWARF info
+  bool invalidatesDWARF() override { return true; }
+
   void run(PassRunner* runner, Module* module) override {
     // Multiple iterations may be necessary: A and B may be identical only after
     // we see the functions C1 and C2 that they call are in fact identical.
diff --git a/src/passes/Flatten.cpp b/src/passes/Flatten.cpp
index 94544ae25..3ff037268 100644
--- a/src/passes/Flatten.cpp
+++ b/src/passes/Flatten.cpp
@@ -51,6 +51,11 @@ struct Flatten
       ExpressionStackWalker<Flatten, UnifiedExpressionVisitor<Flatten>>> {
   bool isFunctionParallel() override { return true; }
 
+  // Flattening splits the original locals into a great many other ones, losing
+  // track of the originals that DWARF refers to.
+  // FIXME DWARF updating does not handle local changes yet.
+  bool invalidatesDWARF() override { return true; }
+
   Pass* create() override { return new Flatten; }
 
   // For each expression, a bunch of expressions that should execute right
diff --git a/src/passes/Inlining.cpp b/src/passes/Inlining.cpp
index 9eade8690..5a2a7137a 100644
--- a/src/passes/Inlining.cpp
+++ b/src/passes/Inlining.cpp
@@ -287,6 +287,10 @@ doInlining(Module* module, Function* into, const InliningAction& action) {
 }
 
 struct Inlining : public Pass {
+  // This pass changes locals and parameters.
+  // FIXME DWARF updating does not handle local changes yet.
+  bool invalidatesDWARF() override { return true; }
+
   // whether to optimize where we inline
   bool optimize = false;
 
diff --git a/src/passes/LocalCSE.cpp b/src/passes/LocalCSE.cpp
index f5cf323ec..7435687ae 100644
--- a/src/passes/LocalCSE.cpp
+++ b/src/passes/LocalCSE.cpp
@@ -52,6 +52,10 @@ namespace wasm {
 struct LocalCSE : public WalkerPass<LinearExecutionWalker<LocalCSE>> {
   bool isFunctionParallel() override { return true; }
 
+  // CSE adds and reuses locals.
+  // FIXME DWARF updating does not handle local changes yet.
+  bool invalidatesDWARF() override { return true; }
+
   Pass* create() override { return new LocalCSE(); }
 
   struct Usable {
diff --git a/src/passes/MergeLocals.cpp b/src/passes/MergeLocals.cpp
index 2223594b6..2b4b3efea 100644
--- a/src/passes/MergeLocals.cpp
+++ b/src/passes/MergeLocals.cpp
@@ -58,6 +58,10 @@ struct MergeLocals
       PostWalker<MergeLocals, UnifiedExpressionVisitor<MergeLocals>>> {
   bool isFunctionParallel() override { return true; }
 
+  // This pass merges locals, mapping the originals to new ones.
+  // FIXME DWARF updating does not handle local changes yet.
+  bool invalidatesDWARF() override { return true; }
+
   Pass* create() override { return new MergeLocals(); }
 
   void doWalkFunction(Function* func) {
diff --git a/src/passes/SSAify.cpp b/src/passes/SSAify.cpp
index df32fc77b..a0bdb4152 100644
--- a/src/passes/SSAify.cpp
+++ b/src/passes/SSAify.cpp
@@ -69,6 +69,10 @@ static LocalSet IMPOSSIBLE_SET;
 struct SSAify : public Pass {
   bool isFunctionParallel() override { return true; }
 
+  // SSAify maps each original local to a number of new ones.
+  // FIXME DWARF updating does not handle local changes yet.
+  bool invalidatesDWARF() override { return true; }
+
   Pass* create() override { return new SSAify(allowMerges); }
 
   SSAify(bool allowMerges) : allowMerges(allowMerges) {}
diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp
index 64e48680e..7d068a7dd 100644
--- a/src/passes/pass.cpp
+++ b/src/passes/pass.cpp
@@ -49,7 +49,7 @@ void PassRegistry::registerPass(const char* name,
 
 std::unique_ptr<Pass> PassRegistry::createPass(std::string name) {
   if (passInfos.find(name) == passInfos.end()) {
-    return nullptr;
+    Fatal() << "Could not find pass: " << name << "\n";
   }
   std::unique_ptr<Pass> ret;
   ret.reset(passInfos[name].create());
@@ -372,144 +372,129 @@ void PassRegistry::registerPasses() {
   //   "lower-i64", "lowers i64 into pairs of i32s", createLowerInt64Pass);
 }
 
+void PassRunner::addIfNoDWARFIssues(std::string passName) {
+  auto pass = PassRegistry::get()->createPass(passName);
+  if (!pass->invalidatesDWARF() ||
+      !Debug::shouldPreserveDWARF(options, *wasm)) {
+    doAdd(std::move(pass));
+  }
+}
+
 void PassRunner::addDefaultOptimizationPasses() {
   addDefaultGlobalOptimizationPrePasses();
   addDefaultFunctionOptimizationPasses();
   addDefaultGlobalOptimizationPostPasses();
 }
 
-// Check whether we should preserve valid DWARF while optimizing. If so, we
-// disable optimizations that currently cause issues with debug info.
-static bool shouldPreserveDWARF(PassOptions& options, Module& wasm) {
-  return options.debugInfo && Debug::hasDWARFSections(wasm);
-}
-
 void PassRunner::addDefaultFunctionOptimizationPasses() {
-  auto preserveDWARF = shouldPreserveDWARF(options, *wasm);
+  // All the additions here are optional if DWARF must be preserved. That is,
+  // when DWARF is relevant we run fewer optimizations.
+  // FIXME: support DWARF in all of them.
+
   // Untangling to semi-ssa form is helpful (but best to ignore merges
   // so as to not introduce new copies).
-  // FIXME DWARF updating does not handle local changes yet.
-  if (!preserveDWARF &&
-      (options.optimizeLevel >= 3 || options.shrinkLevel >= 1)) {
-    add("ssa-nomerge");
+  if (options.optimizeLevel >= 3 || options.shrinkLevel >= 1) {
+    addIfNoDWARFIssues("ssa-nomerge");
   }
   // if we are willing to work very very hard, flatten the IR and do opts
   // that depend on flat IR
-  // FIXME DWARF updating does not handle local changes yet.
-  if (!preserveDWARF && options.optimizeLevel >= 4) {
-    add("flatten");
-    add("local-cse");
-  }
-  add("dce");
-  add("remove-unused-names");
-  add("remove-unused-brs");
-  add("remove-unused-names");
-  add("optimize-instructions");
+  if (options.optimizeLevel >= 4) {
+    addIfNoDWARFIssues("flatten");
+    addIfNoDWARFIssues("local-cse");
+  }
+  addIfNoDWARFIssues("dce");
+  addIfNoDWARFIssues("remove-unused-names");
+  addIfNoDWARFIssues("remove-unused-brs");
+  addIfNoDWARFIssues("remove-unused-names");
+  addIfNoDWARFIssues("optimize-instructions");
   if (options.optimizeLevel >= 2 || options.shrinkLevel >= 2) {
-    add("pick-load-signs");
+    addIfNoDWARFIssues("pick-load-signs");
   }
   // early propagation
   if (options.optimizeLevel >= 3 || options.shrinkLevel >= 2) {
-    add("precompute-propagate");
+    addIfNoDWARFIssues("precompute-propagate");
   } else {
-    add("precompute");
+    addIfNoDWARFIssues("precompute");
   }
   if (options.lowMemoryUnused) {
     if (options.optimizeLevel >= 3 || options.shrinkLevel >= 1) {
-      add("optimize-added-constants-propagate");
+      addIfNoDWARFIssues("optimize-added-constants-propagate");
     } else {
-      add("optimize-added-constants");
+      addIfNoDWARFIssues("optimize-added-constants");
     }
   }
   if (options.optimizeLevel >= 2 || options.shrinkLevel >= 2) {
-    add("code-pushing");
+    addIfNoDWARFIssues("code-pushing");
   }
   // don't create if/block return values yet, as coalesce can remove copies that
   // that could inhibit
-  add("simplify-locals-nostructure");
-  add("vacuum"); // previous pass creates garbage
-  add("reorder-locals");
+  addIfNoDWARFIssues("simplify-locals-nostructure");
+  addIfNoDWARFIssues("vacuum"); // previous pass creates garbage
+  addIfNoDWARFIssues("reorder-locals");
   // simplify-locals opens opportunities for optimizations
-  add("remove-unused-brs");
+  addIfNoDWARFIssues("remove-unused-brs");
   // if we are willing to work hard, also optimize copies before coalescing
-  // FIXME DWARF updating does not handle local changes yet.
-  if (!preserveDWARF &&
-      (options.optimizeLevel >= 3 || options.shrinkLevel >= 2)) {
-    add("merge-locals"); // very slow on e.g. sqlite
-  }
-  // FIXME DWARF updating does not handle local changes yet.
-  if (!preserveDWARF) {
-    add("coalesce-locals");
-  }
-  add("simplify-locals");
-  add("vacuum");
-  add("reorder-locals");
-  // FIXME DWARF updating does not handle local changes yet.
-  if (!preserveDWARF) {
-    add("coalesce-locals");
-    add("reorder-locals");
-  }
-  add("vacuum");
+  if (options.optimizeLevel >= 3 || options.shrinkLevel >= 2) {
+    addIfNoDWARFIssues("merge-locals"); // very slow on e.g. sqlite
+  }
+  addIfNoDWARFIssues("coalesce-locals");
+  addIfNoDWARFIssues("simplify-locals");
+  addIfNoDWARFIssues("vacuum");
+  addIfNoDWARFIssues("reorder-locals");
+  addIfNoDWARFIssues("coalesce-locals");
+  addIfNoDWARFIssues("reorder-locals");
+  addIfNoDWARFIssues("vacuum");
   if (options.optimizeLevel >= 3 || options.shrinkLevel >= 1) {
-    add("code-folding");
-  }
-  add("merge-blocks");        // makes remove-unused-brs more effective
-  add("remove-unused-brs");   // coalesce-locals opens opportunities
-  add("remove-unused-names"); // remove-unused-brs opens opportunities
-  add("merge-blocks");        // clean up remove-unused-brs new blocks
+    addIfNoDWARFIssues("code-folding");
+  }
+  addIfNoDWARFIssues("merge-blocks"); // makes remove-unused-brs more effective
+  addIfNoDWARFIssues(
+    "remove-unused-brs"); // coalesce-locals opens opportunities
+  addIfNoDWARFIssues(
+    "remove-unused-names");           // remove-unused-brs opens opportunities
+  addIfNoDWARFIssues("merge-blocks"); // clean up remove-unused-brs new blocks
   // late propagation
   if (options.optimizeLevel >= 3 || options.shrinkLevel >= 2) {
-    add("precompute-propagate");
+    addIfNoDWARFIssues("precompute-propagate");
   } else {
-    add("precompute");
+    addIfNoDWARFIssues("precompute");
   }
-  add("optimize-instructions");
+  addIfNoDWARFIssues("optimize-instructions");
   if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) {
-    add("rse"); // after all coalesce-locals, and before a final vacuum
+    addIfNoDWARFIssues(
+      "rse"); // after all coalesce-locals, and before a final vacuum
   }
-  add("vacuum"); // just to be safe
+  addIfNoDWARFIssues("vacuum"); // just to be safe
 }
 
 void PassRunner::addDefaultGlobalOptimizationPrePasses() {
-  // FIXME DWARF updating does not handle merging debug info with merged code.
-  if (!shouldPreserveDWARF(options, *wasm)) {
-    add("duplicate-function-elimination");
-  }
-  add("memory-packing");
+  addIfNoDWARFIssues("duplicate-function-elimination");
+  addIfNoDWARFIssues("memory-packing");
 }
 
 void PassRunner::addDefaultGlobalOptimizationPostPasses() {
-  auto preserveDWARF = shouldPreserveDWARF(options, *wasm);
-  // FIXME DWARF may be badly affected currently as DAE changes function
-  // signatures and hence params and locals.
-  if (!preserveDWARF &&
-      (options.optimizeLevel >= 2 || options.shrinkLevel >= 1)) {
-    add("dae-optimizing");
-  }
-  // FIXME DWARF updating does not handle inlining yet.
-  if (!preserveDWARF &&
-      (options.optimizeLevel >= 2 || options.shrinkLevel >= 2)) {
-    add("inlining-optimizing");
+  if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) {
+    addIfNoDWARFIssues("dae-optimizing");
   }
-  // Optimizations show more functions as duplicate, so run this here in Post.
-  // FIXME DWARF updating does not handle merging debug info with merged code.
-  if (!preserveDWARF) {
-    add("duplicate-function-elimination");
+  if (options.optimizeLevel >= 2 || options.shrinkLevel >= 2) {
+    addIfNoDWARFIssues("inlining-optimizing");
   }
-  add("duplicate-import-elimination");
+  // Optimizations show more functions as duplicate, so run this here in Post.
+  addIfNoDWARFIssues("duplicate-function-elimination");
+  addIfNoDWARFIssues("duplicate-import-elimination");
   if (options.optimizeLevel >= 2 || options.shrinkLevel >= 2) {
-    add("simplify-globals-optimizing");
+    addIfNoDWARFIssues("simplify-globals-optimizing");
   } else {
-    add("simplify-globals");
+    addIfNoDWARFIssues("simplify-globals");
   }
-  add("remove-unused-module-elements");
+  addIfNoDWARFIssues("remove-unused-module-elements");
   // may allow more inlining/dae/etc., need --converge for that
-  add("directize");
+  addIfNoDWARFIssues("directize");
   // perform Stack IR optimizations here, at the very end of the
   // optimization pipeline
   if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) {
-    add("generate-stack-ir");
-    add("optimize-stack-ir");
+    addIfNoDWARFIssues("generate-stack-ir");
+    addIfNoDWARFIssues("optimize-stack-ir");
   }
 }
 
@@ -670,6 +655,10 @@ void PassRunner::runOnFunction(Function* func) {
 }
 
 void PassRunner::doAdd(std::unique_ptr<Pass> pass) {
+  if (Debug::shouldPreserveDWARF(options, *wasm) && pass->invalidatesDWARF()) {
+    std::cerr << "warning: running pass '" << pass->name
+              << "' which is not fully compatible with DWARF\n";
+  }
   passes.emplace_back(std::move(pass));
 }
 
diff --git a/src/wasm-debug.h b/src/wasm-debug.h
index 009edb555..d5ab337d4 100644
--- a/src/wasm-debug.h
+++ b/src/wasm-debug.h
@@ -23,6 +23,7 @@
 
 #include <string>
 
+#include "pass.h"
 #include "wasm.h"
 
 namespace wasm {
@@ -36,6 +37,10 @@ bool hasDWARFSections(const Module& wasm);
 // Dump the DWARF sections to stdout.
 void dumpDWARF(const Module& wasm);
 
+// Check whether we should preserve valid DWARF while optimizing. (If so, we
+// will disable optimizations that currently cause issues with debug info.)
+bool shouldPreserveDWARF(PassOptions& options, Module& wasm);
+
 // Update the DWARF sections.
 void writeDWARFSections(Module& wasm, const BinaryLocations& newLocations);
 
diff --git a/src/wasm/wasm-debug.cpp b/src/wasm/wasm-debug.cpp
index 2c5ed9515..867f4b14b 100644
--- a/src/wasm/wasm-debug.cpp
+++ b/src/wasm/wasm-debug.cpp
@@ -86,6 +86,10 @@ void dumpDWARF(const Module& wasm) {
   info.context->dump(llvm::outs(), options);
 }
 
+bool shouldPreserveDWARF(PassOptions& options, Module& wasm) {
+  return options.debugInfo && hasDWARFSections(wasm);
+}
+
 //
 // Big picture: We use a DWARFContext to read data, then DWARFYAML support
 // code to write it. That is not the main LLVM Dwarf code used for writing
@@ -1091,6 +1095,8 @@ void writeDWARFSections(Module& wasm, const BinaryLocations& newLocations) {
   std::cerr << "warning: no DWARF updating support present\n";
 }
 
+bool shouldPreserveDWARF(PassOptions& options, Module& wasm) { return false; }
+
 #endif // BUILD_LLVM_DWARF
 
 } // namespace Debug