From 9a645a99c6db96a561158767b088e4eae3a42ea5 Mon Sep 17 00:00:00 2001
From: Sam Clegg <sbc@chromium.org>
Date: Thu, 5 Dec 2019 11:24:01 -0800
Subject: [PATCH] Fix renaming in FixInvokeFunctionNamesWalker

This fixes https://github.com/emscripten-core/emscripten/issues/9950.
The issue only shows up when debug names are not present so most of
the changes in CL come from disabling debug names in the lld tests.
We want to make sure that wasm-emscripten-finalize runs fine without
debug names so I think it makes most sense to test in this mode.

The actual bugfix is in wasm-emscripten.cpp as part of the
FixInvokeFunctionNamesWalker.  The problem was the name of the function
rather than is import name was being added to importRenames.  This means
that when debug names were present (and the two names were the same)
we didn't see the bug.
---
 scripts/test/generate_lld_tests.py | 16 ++++++----
 scripts/test/shared.py             |  7 ++---
 src/wasm/wasm-emscripten.cpp       | 21 ++++++++-----
 test/lld/shared_longjmp.wat        | 48 +++++++++++++++---------------
 test/lld/shared_longjmp.wat.out    | 46 ++++++++++++++--------------
 5 files changed, 73 insertions(+), 65 deletions(-)

diff --git a/scripts/test/generate_lld_tests.py b/scripts/test/generate_lld_tests.py
index 64fcff2ac01..005e09f1506 100755
--- a/scripts/test/generate_lld_tests.py
+++ b/scripts/test/generate_lld_tests.py
@@ -30,8 +30,8 @@ def files_with_extensions(path, extensions):
             yield file, ext
 
 
-def generate_wast_files(llvm_bin, emscripten_root):
-    print('\n[ building wast files from C sources... ]\n')
+def generate_wat_files(llvm_bin, emscripten_root):
+    print('\n[ building wat files from C sources... ]\n')
 
     lld_path = os.path.join(shared.options.binaryen_test, 'lld')
     for src_file, ext in files_with_extensions(lld_path, ['.c', '.cpp']):
@@ -42,11 +42,11 @@ def generate_wast_files(llvm_bin, emscripten_root):
         obj_path = os.path.join(lld_path, obj_file)
 
         wasm_file = src_file.replace(ext, '.wasm')
-        wast_file = src_file.replace(ext, '.wast')
+        wat_file = src_file.replace(ext, '.wat')
 
         obj_path = os.path.join(lld_path, obj_file)
         wasm_path = os.path.join(lld_path, wasm_file)
-        wast_path = os.path.join(lld_path, wast_file)
+        wat_path = os.path.join(lld_path, wat_file)
         is_shared = 'shared' in src_file
 
         compile_cmd = [
@@ -70,6 +70,10 @@ def generate_wast_files(llvm_bin, emscripten_root):
             '--export', '__data_end',
             '--global-base=568',
         ]
+        # We had a regression where this test only worked if debug names
+        # were included.
+        if 'shared_longjmp' in src_file:
+            link_cmd.append('--strip-debug')
         if is_shared:
             compile_cmd.append('-fPIC')
             compile_cmd.append('-fvisibility=default')
@@ -80,7 +84,7 @@ def generate_wast_files(llvm_bin, emscripten_root):
         try:
             support.run_command(compile_cmd)
             support.run_command(link_cmd)
-            support.run_command(shared.WASM_DIS + [wasm_path, '-o', wast_path])
+            support.run_command(shared.WASM_DIS + [wasm_path, '-o', wat_path])
         finally:
             # Don't need the .o or .wasm files, don't leave them around
             shared.delete_from_orbit(obj_path)
@@ -91,4 +95,4 @@ def generate_wast_files(llvm_bin, emscripten_root):
     if len(shared.options.positional_args) != 2:
         print('Usage: generate_lld_tests.py [llvm/bin/dir] [path/to/emscripten]')
         sys.exit(1)
-    generate_wast_files(*shared.options.positional_args)
+    generate_wat_files(*shared.options.positional_args)
diff --git a/scripts/test/shared.py b/scripts/test/shared.py
index 1ec42bc59d2..44a8dcc5204 100644
--- a/scripts/test/shared.py
+++ b/scripts/test/shared.py
@@ -87,6 +87,7 @@ def parse_args(args):
 
 options = parse_args(sys.argv[1:])
 requested = options.positional_args
+script_dir = os.path.dirname(os.path.abspath(__file__))
 
 num_failures = 0
 warnings = []
@@ -123,8 +124,7 @@ def warn(text):
 
 # Locate Binaryen source directory if not specified.
 if not options.binaryen_root:
-    path_parts = os.path.abspath(__file__).split(os.path.sep)
-    options.binaryen_root = os.path.sep.join(path_parts[:-3])
+    options.binaryen_root = os.path.dirname(os.path.dirname(script_dir))
 
 options.binaryen_test = os.path.join(options.binaryen_root, 'test')
 
@@ -207,8 +207,7 @@ def wrap_with_valgrind(cmd):
 
 
 def in_binaryen(*args):
-    __rootpath__ = os.path.abspath(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))
-    return os.path.join(__rootpath__, *args)
+    return os.path.join(options.binaryen_root, *args)
 
 
 os.environ['BINARYEN'] = in_binaryen()
diff --git a/src/wasm/wasm-emscripten.cpp b/src/wasm/wasm-emscripten.cpp
index f069e558def..ae7e7f8715b 100644
--- a/src/wasm/wasm-emscripten.cpp
+++ b/src/wasm/wasm-emscripten.cpp
@@ -996,8 +996,9 @@ struct FixInvokeFunctionNamesWalker
   std::vector<Name> toRemove;
   std::set<Name> newImports;
   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,15 +1053,18 @@ struct FixInvokeFunctionNamesWalker
       return;
     }
 
-    assert(importRenames.count(curr->name) == 0);
-    BYN_TRACE("renaming: " << curr->name << " -> " << newname << "\n");
-    importRenames[curr->name] = newname;
+    BYN_TRACE("renaming import: " << curr->module << "." << curr->base << " ("
+                                  << curr->name << ") -> " << newname << "\n");
+    assert(importRenames.count(curr->base) == 0);
+    importRenames[curr->base] = newname;
     // Either rename or remove the existing import
-    if (wasm.getFunctionOrNull(newname) || !newImports.insert(newname).second) {
+    if (imports.getImportedFunction(curr->module, newname) ||
+        !newImports.insert(newname).second) {
+      BYN_TRACE("using existing import\n");
       toRemove.push_back(curr->name);
     } else {
+      BYN_TRACE("renaming import\n");
       curr->base = newname;
-      curr->name = newname;
     }
   }
 
@@ -1069,10 +1073,11 @@ struct FixInvokeFunctionNamesWalker
       wasm.removeFunction(importName);
     }
     ModuleUtils::renameFunctions(wasm, importRenames);
-    ImportInfo imports(wasm);
+    // Update any associated GOT.func imports.
     for (auto& pair : importRenames) {
-      // Update any associated GOT.func import.
       if (auto g = imports.getImportedGlobal("GOT.func", pair.first)) {
+        BYN_TRACE("renaming corresponding GOT entry: " << g->base << " -> "
+                                                       << pair.second << "\n");
         g->base = pair.second;
       }
     }
diff --git a/test/lld/shared_longjmp.wat b/test/lld/shared_longjmp.wat
index 235e1fadf14..b5179676ccf 100644
--- a/test/lld/shared_longjmp.wat
+++ b/test/lld/shared_longjmp.wat
@@ -15,41 +15,41 @@
  (import "GOT.mem" "__THREW__" (global $gimport$13 (mut i32)))
  (import "GOT.func" "emscripten_longjmp_jmpbuf" (global $gimport$14 (mut i32)))
  (import "GOT.mem" "__threwValue" (global $gimport$15 (mut i32)))
- (import "env" "malloc" (func $malloc (param i32) (result i32)))
- (import "env" "saveSetjmp" (func $saveSetjmp (param i32 i32 i32 i32) (result i32)))
- (import "env" "getTempRet0" (func $getTempRet0 (result i32)))
- (import "env" "emscripten_longjmp_jmpbuf" (func $emscripten_longjmp_jmpbuf (param i32 i32)))
- (import "env" "__invoke_void_i32_i32" (func $__invoke_void_i32_i32 (param i32 i32 i32)))
- (import "env" "testSetjmp" (func $testSetjmp (param i32 i32 i32) (result i32)))
- (import "env" "setTempRet0" (func $setTempRet0 (param i32)))
- (import "env" "free" (func $free (param i32)))
- (import "env" "emscripten_longjmp" (func $emscripten_longjmp (param i32 i32)))
+ (import "env" "malloc" (func $fimport$4 (param i32) (result i32)))
+ (import "env" "saveSetjmp" (func $fimport$5 (param i32 i32 i32 i32) (result i32)))
+ (import "env" "getTempRet0" (func $fimport$6 (result i32)))
+ (import "env" "emscripten_longjmp_jmpbuf" (func $fimport$7 (param i32 i32)))
+ (import "env" "__invoke_void_i32_i32" (func $fimport$8 (param i32 i32 i32)))
+ (import "env" "testSetjmp" (func $fimport$9 (param i32 i32 i32) (result i32)))
+ (import "env" "setTempRet0" (func $fimport$10 (param i32)))
+ (import "env" "free" (func $fimport$11 (param i32)))
+ (import "env" "emscripten_longjmp" (func $fimport$12 (param i32 i32)))
  (global $global$0 i32 (i32.const 0))
  (global $global$1 i32 (i32.const 4))
- (export "__wasm_call_ctors" (func $__wasm_call_ctors))
- (export "_start" (func $_start))
+ (export "__wasm_call_ctors" (func $0))
+ (export "_start" (func $2))
  (export "__THREW__" (global $global$0))
  (export "__threwValue" (global $global$1))
- (func $__wasm_call_ctors (; 9 ;) (type $7)
-  (call $__wasm_apply_relocs)
+ (func $0 (; 9 ;) (type $7)
+  (call $1)
  )
- (func $__wasm_apply_relocs (; 10 ;) (type $7)
+ (func $1 (; 10 ;) (type $7)
  )
- (func $_start (; 11 ;) (type $7)
+ (func $2 (; 11 ;) (type $7)
   (local $0 i32)
   (local $1 i32)
   (local $2 i32)
   (local $3 i32)
   (i32.store
    (local.tee $0
-    (call $malloc
+    (call $fimport$4
      (i32.const 40)
     )
    )
    (i32.const 0)
   )
   (local.set $1
-   (call $saveSetjmp
+   (call $fimport$5
     (local.get $0)
     (i32.const 1)
     (local.get $0)
@@ -57,7 +57,7 @@
    )
   )
   (local.set $2
-   (call $getTempRet0)
+   (call $fimport$6)
   )
   (local.set $0
    (i32.const 0)
@@ -74,7 +74,7 @@
       )
       (i32.const 0)
      )
-     (call $__invoke_void_i32_i32
+     (call $fimport$8
       (global.get $gimport$14)
       (local.get $0)
       (i32.const 1)
@@ -108,7 +108,7 @@
       )
       (br_if $label$1
        (i32.eqz
-        (call $testSetjmp
+        (call $fimport$9
          (i32.load
           (local.get $3)
          )
@@ -117,22 +117,22 @@
         )
        )
       )
-      (call $setTempRet0
+      (call $fimport$10
        (local.get $0)
       )
      )
      (local.set $0
-      (call $getTempRet0)
+      (call $fimport$6)
      )
      (br $label$3)
     )
    )
-   (call $free
+   (call $fimport$11
     (local.get $1)
    )
    (return)
   )
-  (call $emscripten_longjmp
+  (call $fimport$12
    (local.get $3)
    (local.get $0)
   )
diff --git a/test/lld/shared_longjmp.wat.out b/test/lld/shared_longjmp.wat.out
index 083fcb3ccd9..871e26b27ee 100644
--- a/test/lld/shared_longjmp.wat.out
+++ b/test/lld/shared_longjmp.wat.out
@@ -12,14 +12,14 @@
  (import "env" "table" (table $0 0 funcref))
  (import "env" "__memory_base" (global $gimport$2 i32))
  (import "env" "__table_base" (global $gimport$3 i32))
- (import "env" "malloc" (func $malloc (param i32) (result i32)))
- (import "env" "saveSetjmp" (func $saveSetjmp (param i32 i32 i32 i32) (result i32)))
- (import "env" "getTempRet0" (func $getTempRet0 (result i32)))
- (import "env" "invoke_vii" (func $invoke_vii (param i32 i32 i32)))
- (import "env" "testSetjmp" (func $testSetjmp (param i32 i32 i32) (result i32)))
- (import "env" "setTempRet0" (func $setTempRet0 (param i32)))
- (import "env" "free" (func $free (param i32)))
- (import "env" "emscripten_longjmp" (func $emscripten_longjmp (param i32 i32)))
+ (import "env" "malloc" (func $fimport$4 (param i32) (result i32)))
+ (import "env" "saveSetjmp" (func $fimport$5 (param i32 i32 i32 i32) (result i32)))
+ (import "env" "getTempRet0" (func $fimport$6 (result i32)))
+ (import "env" "invoke_vii" (func $fimport$8 (param i32 i32 i32)))
+ (import "env" "testSetjmp" (func $fimport$9 (param i32 i32 i32) (result i32)))
+ (import "env" "setTempRet0" (func $fimport$10 (param i32)))
+ (import "env" "free" (func $fimport$11 (param i32)))
+ (import "env" "emscripten_longjmp" (func $fimport$12 (param i32 i32)))
  (import "env" "g$__THREW__" (func $g$__THREW__ (result i32)))
  (import "env" "g$__threwValue" (func $g$__threwValue (result i32)))
  (import "env" "fp$emscripten_longjmp$vii" (func $fp$emscripten_longjmp$vii (result i32)))
@@ -28,32 +28,32 @@
  (global $gimport$15 (mut i32) (i32.const 0))
  (global $global$0 i32 (i32.const 0))
  (global $global$1 i32 (i32.const 4))
- (export "_start" (func $_start))
+ (export "_start" (func $2))
  (export "__THREW__" (global $global$0))
  (export "__threwValue" (global $global$1))
  (export "dynCall_vii" (func $dynCall_vii))
  (export "__post_instantiate" (func $__post_instantiate))
- (func $__wasm_call_ctors (; 11 ;)
-  (call $__wasm_apply_relocs)
+ (func $0 (; 11 ;)
+  (call $1)
  )
- (func $__wasm_apply_relocs (; 12 ;)
+ (func $1 (; 12 ;)
   (nop)
  )
- (func $_start (; 13 ;)
+ (func $2 (; 13 ;)
   (local $0 i32)
   (local $1 i32)
   (local $2 i32)
   (local $3 i32)
   (i32.store
    (local.tee $0
-    (call $malloc
+    (call $fimport$4
      (i32.const 40)
     )
    )
    (i32.const 0)
   )
   (local.set $1
-   (call $saveSetjmp
+   (call $fimport$5
     (local.get $0)
     (i32.const 1)
     (local.get $0)
@@ -61,7 +61,7 @@
    )
   )
   (local.set $2
-   (call $getTempRet0)
+   (call $fimport$6)
   )
   (local.set $0
    (i32.const 0)
@@ -78,7 +78,7 @@
       )
       (i32.const 0)
      )
-     (call $invoke_vii
+     (call $fimport$8
       (global.get $gimport$14)
       (local.get $0)
       (i32.const 1)
@@ -112,7 +112,7 @@
       )
       (br_if $label$1
        (i32.eqz
-        (call $testSetjmp
+        (call $fimport$9
          (i32.load
           (local.get $3)
          )
@@ -121,22 +121,22 @@
         )
        )
       )
-      (call $setTempRet0
+      (call $fimport$10
        (local.get $0)
       )
      )
      (local.set $0
-      (call $getTempRet0)
+      (call $fimport$6)
      )
      (br $label$3)
     )
    )
-   (call $free
+   (call $fimport$11
     (local.get $1)
    )
    (return)
   )
-  (call $emscripten_longjmp
+  (call $fimport$12
    (local.get $3)
    (local.get $0)
   )
@@ -151,7 +151,7 @@
  )
  (func $__post_instantiate (; 15 ;)
   (call $__assign_got_enties)
-  (call $__wasm_call_ctors)
+  (call $0)
  )
  (func $__assign_got_enties (; 16 ;)
   (global.set $gimport$13