From f62e171c38bea14302f9b79f7941a248ea704425 Mon Sep 17 00:00:00 2001
From: Sam Clegg <sbc@chromium.org>
Date: Fri, 20 Dec 2019 16:02:34 -0800
Subject: 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.
---
 src/wasm/wasm-emscripten.cpp | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

(limited to 'src')

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;
       }
     }
-- 
cgit v1.2.3