Reland "Fix renaming in FixInvokeFunctionNamesWalker (#2513)" (#2542)

* Reland "Fix renaming in FixInvokeFunctionNamesWalker (#2513)"

In the previous iteration of this change we were not calling
`renameFunctions` for each of the functions we removed.

The problem manifested itself when we rename the imported function to
`emscripten_longjmp_jmpbuf` to `emscripten_longjmp`.  In this case the
import of `emscripten_longjmp` already exists so we remove the import of
`emscripten_longjmp_jmpbuf` but we were not correclty calling
renameFunctions to handle the rename of all the uses.

Add an additional test case to cover the failures that we saw on the
emscripten tree.

1 files changed, 26 insertions, 15 deletions

diff --git a/src/wasm/wasm-emscripten.cpp b/src/wasm/wasm-emscripten.cpp
index f069e558d..3d65f9de2 100644
--- a/src/wasm/wasm-emscripten.cpp
+++ b/src/wasm/wasm-emscripten.cpp
@@ -993,11 +993,11 @@ struct FixInvokeFunctionNamesWalker
   : public PostWalker<FixInvokeFunctionNamesWalker> {
   Module& wasm;
   std::map<Name, Name> importRenames;
-  std::vector<Name> toRemove;
-  std::set<Name> newImports;
+  std::map<Name, Name> functionReplace;
   std::set<Signature> invokeSigs;
+  ImportInfo imports;
 
-  FixInvokeFunctionNamesWalker(Module& _wasm) : wasm(_wasm) {}
+  FixInvokeFunctionNamesWalker(Module& _wasm) : wasm(_wasm), imports(wasm) {}
 
   // Converts invoke wrapper names generated by LLVM backend to real invoke
   // wrapper names that are expected by JavaScript glue code.
@@ -1052,27 +1052,38 @@ struct FixInvokeFunctionNamesWalker
       return;
     }
 
-    assert(importRenames.count(curr->name) == 0);
-    BYN_TRACE("renaming: " << curr->name << " -> " << newname << "\n");
-    importRenames[curr->name] = newname;
-    // Either rename or remove the existing import
-    if (wasm.getFunctionOrNull(newname) || !newImports.insert(newname).second) {
-      toRemove.push_back(curr->name);
+    BYN_TRACE("renaming import: " << curr->module << "." << curr->base << " ("
+                                  << curr->name << ") -> " << newname << "\n");
+    assert(importRenames.count(curr->base) == 0);
+    importRenames[curr->base] = newname;
+    // Either rename the import, or replace it with an existing one
+    Function* existingFunc = imports.getImportedFunction(curr->module, newname);
+    if (existingFunc) {
+      BYN_TRACE("replacing with an existing import: " << existingFunc->name
+                                                      << "\n");
+      functionReplace[curr->name] = existingFunc->name;
     } else {
+      BYN_TRACE("renaming the import in place\n");
       curr->base = newname;
-      curr->name = newname;
     }
   }
 
   void visitModule(Module* curr) {
-    for (auto importName : toRemove) {
-      wasm.removeFunction(importName);
+    // For each replaced function first remove the function itself then
+    // rename all uses to the point to the new function.
+    for (auto& pair : functionReplace) {
+      BYN_TRACE("removeFunction " << pair.first << "\n");
+      wasm.removeFunction(pair.first);
     }
-    ModuleUtils::renameFunctions(wasm, importRenames);
-    ImportInfo imports(wasm);
+    // Rename all uses of the removed functions
+    ModuleUtils::renameFunctions(wasm, functionReplace);
+
+    // For imports that for renamed, update any associated GOT.func imports.
     for (auto& pair : importRenames) {
-      // Update any associated GOT.func import.
+      BYN_TRACE("looking for: GOT.func." << pair.first << "\n");
       if (auto g = imports.getImportedGlobal("GOT.func", pair.first)) {
+        BYN_TRACE("renaming corresponding GOT entry: " << g->base << " -> "
+                                                       << pair.second << "\n");
         g->base = pair.second;
       }
     }