Do not attempt to preserve DWARF if a previous pass removes it (#3887)

If we run a pass that removes DWARF followed by one that could destroy it, then
there is no possible problem - there is nothing left to destroy. We can run the later
pass with no issues (and no warnings).

Also add an assertion on running a pass runner only once. That has always been
the assumption, and now that we track whether the added passes remove debug
info, we need to check it.

Fixes emscripten-core/emscripten#14161

4 files changed, 65 insertions, 6 deletions

diff --git a/src/pass.h b/src/pass.h
index dd7c67bc1..0be45e2d1 100644
--- a/src/pass.h
+++ b/src/pass.h
@@ -256,10 +256,20 @@ struct PassRunner {
   //  3: like 1, and also dumps out byn-* files for each pass as it is run.
   static int getPassDebug();
 
-protected:
-  bool isNested = false;
+  // Returns whether a pass by that name will remove debug info.
+  static bool passRemovesDebugInfo(const std::string& name);
 
 private:
+  // Whether this is a nested pass runner.
+  bool isNested = false;
+
+  // Whether the passes we have added so far to be run (but not necessarily run
+  // yet) have removed DWARF.
+  bool addedPassesRemovedDWARF = false;
+
+  // Whether this pass runner has run. A pass runner should only be run once.
+  bool ran = false;
+
   void doAdd(std::unique_ptr<Pass> pass);
 
   void runPass(Pass* pass);
@@ -272,6 +282,8 @@ private:
   // If a function is passed, we operate just on that function;
   // otherwise, the whole module.
   void handleAfterEffects(Pass* pass, Function* func = nullptr);
+
+  bool shouldPreserveDWARF();
 };
 
 //
diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp
index 260bc761b..89b367272 100644
--- a/src/passes/pass.cpp
+++ b/src/passes/pass.cpp
@@ -386,8 +386,7 @@ void PassRegistry::registerPasses() {
 
 void PassRunner::addIfNoDWARFIssues(std::string passName) {
   auto pass = PassRegistry::get()->createPass(passName);
-  if (!pass->invalidatesDWARF() ||
-      !Debug::shouldPreserveDWARF(options, *wasm)) {
+  if (!pass->invalidatesDWARF() || !shouldPreserveDWARF()) {
     doAdd(std::move(pass));
   }
 }
@@ -533,6 +532,9 @@ static void dumpWast(Name name, Module* wasm) {
 }
 
 void PassRunner::run() {
+  assert(!ran);
+  ran = true;
+
   static const int passDebug = getPassDebug();
   // Emit logging information when asked for. At passDebug level 1+ we log
   // the main passes, while in 2 we also log nested ones. Note that for
@@ -670,10 +672,13 @@ void PassRunner::runOnFunction(Function* func) {
 }
 
 void PassRunner::doAdd(std::unique_ptr<Pass> pass) {
-  if (Debug::shouldPreserveDWARF(options, *wasm) && pass->invalidatesDWARF()) {
+  if (pass->invalidatesDWARF() && shouldPreserveDWARF()) {
     std::cerr << "warning: running pass '" << pass->name
               << "' which is not fully compatible with DWARF\n";
   }
+  if (passRemovesDebugInfo(pass->name)) {
+    addedPassesRemovedDWARF = true;
+  }
   passes.emplace_back(std::move(pass));
 }
 
@@ -820,4 +825,23 @@ int PassRunner::getPassDebug() {
   return passDebug;
 }
 
+bool PassRunner::passRemovesDebugInfo(const std::string& name) {
+  return name == "strip" || name == "strip-debug" || name == "strip-dwarf";
+}
+
+bool PassRunner::shouldPreserveDWARF() {
+  // Check if the debugging subsystem wants to preserve DWARF.
+  if (!Debug::shouldPreserveDWARF(options, *wasm)) {
+    return false;
+  }
+
+  // We may need DWARF. Check if one of our previous passes would remove it
+  // anyhow, in which case, there is nothing to preserve.
+  if (addedPassesRemovedDWARF) {
+    return false;
+  }
+
+  return true;
+}
+
 } // namespace wasm
diff --git a/src/tools/wasm-opt.cpp b/src/tools/wasm-opt.cpp
index 8884619fe..45d394162 100644
--- a/src/tools/wasm-opt.cpp
+++ b/src/tools/wasm-opt.cpp
@@ -61,7 +61,7 @@ static std::string runCommand(std::string command) {
 
 static bool willRemoveDebugInfo(const std::vector<std::string>& passes) {
   for (auto& pass : passes) {
-    if (pass == "strip" || pass == "strip-debug" || pass == "strip-dwarf") {
+    if (PassRunner::passRemovesDebugInfo(pass)) {
       return true;
     }
   }
diff --git a/test/unit/test_dwarf.py b/test/unit/test_dwarf.py
index 37abf6b18..809c4af05 100644
--- a/test/unit/test_dwarf.py
+++ b/test/unit/test_dwarf.py
@@ -26,3 +26,26 @@ class DWARFTest(utils.BinaryenTestCase):
         # safe passes do not
         err = shared.run_process(shared.WASM_OPT + args + ['--metrics'], stderr=subprocess.PIPE).stderr
         self.assertNotIn(warning, err)
+
+    def test_strip_dwarf_and_opts(self):
+        # some optimizations are disabled when DWARF is present (as they would
+        # destroy it). we scan the wasm to see if there is any DWARF when
+        # making the decision whether to run them. this test checks that we also
+        # check if --strip* is being run, which would remove the DWARF anyhow
+        path = self.input_path(os.path.join('dwarf', 'cubescript.wasm'))
+        # strip the DWARF, then run all the opts to check as much as possible
+        args = [path, '--strip-dwarf', '-Oz']
+        # run it normally, without -g. in this case no DWARF will be preserved
+        # in a trivial way
+        shared.run_process(shared.WASM_OPT + args + ['-o', 'a.wasm'])
+        # run it with -g. in this case we need to be clever as described above,
+        # and see --strip-dwarf removes the need for DWARF
+        shared.run_process(shared.WASM_OPT + args + ['-o', 'b.wasm', '-g'])
+        # run again on the last output without -g, as we don't want the names
+        # section to skew the results
+        shared.run_process(shared.WASM_OPT + ['b.wasm', '-o', 'c.wasm'])
+        # compare the sizes. there might be a tiny difference in size to to
+        # minor roundtrip changes, so ignore up to a tiny %
+        a_size = os.path.getsize('a.wasm')
+        c_size = os.path.getsize('c.wasm')
+        self.assertLess((100 * abs(a_size - c_size)) / c_size, 1)