Reland "Fix renaming in FixInvokeFunctionNamesWalker (#2513)"

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 '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)

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')
 
@@ -206,8 +206,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()

src/wasm/wasm-emscripten.cpp

@@ -994,11 +994,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.
@@ -1053,27 +1053,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;
       }
     }

test/lld/longjmp.c

@@ -0,0 +1,17 @@
+typedef struct jmp_buf_buf {
+  int thing;
+} jmp_buf;
+
+void longjmp(jmp_buf env, int val);
+int setjmp(jmp_buf env);
+
+int __THREW__;
+int __threwValue;
+
+int main() {
+  jmp_buf jmp;
+  if (setjmp(jmp) == 0) {
+    longjmp(jmp, 1);
+  }
+  return 0;
+}

test/lld/longjmp.wat

@@ -0,0 +1,136 @@
+(module
+ (type $i32_=>_none (func (param i32)))
+ (type $i32_i32_=>_none (func (param i32 i32)))
+ (type $none_=>_i32 (func (result i32)))
+ (type $none_=>_none (func))
+ (type $i32_i32_i32_=>_none (func (param i32 i32 i32)))
+ (type $i32_=>_i32 (func (param i32) (result i32)))
+ (type $i32_i32_=>_i32 (func (param i32 i32) (result i32)))
+ (type $i32_i32_i32_=>_i32 (func (param i32 i32 i32) (result i32)))
+ (type $i32_i32_i32_i32_=>_i32 (func (param i32 i32 i32 i32) (result i32)))
+ (import "env" "malloc" (func $fimport$0 (param i32) (result i32)))
+ (import "env" "saveSetjmp" (func $fimport$1 (param i32 i32 i32 i32) (result i32)))
+ (import "env" "getTempRet0" (func $fimport$2 (result i32)))
+ (import "env" "emscripten_longjmp_jmpbuf" (func $fimport$3 (param i32 i32)))
+ (import "env" "__invoke_void_i32_i32" (func $fimport$4 (param i32 i32 i32)))
+ (import "env" "testSetjmp" (func $fimport$5 (param i32 i32 i32) (result i32)))
+ (import "env" "setTempRet0" (func $fimport$6 (param i32)))
+ (import "env" "free" (func $fimport$7 (param i32)))
+ (import "env" "emscripten_longjmp" (func $fimport$8 (param i32 i32)))
+ (memory $0 2)
+ (table $0 2 2 funcref)
+ (elem (i32.const 1) $fimport$3)
+ (global $global$0 (mut i32) (i32.const 66112))
+ (global $global$1 i32 (i32.const 576))
+ (export "memory" (memory $0))
+ (export "__wasm_call_ctors" (func $0))
+ (export "main" (func $2))
+ (export "__data_end" (global $global$1))
+ (func $0 (; 9 ;)
+ )
+ (func $1 (; 10 ;) (result i32)
+  (local $0 i32)
+  (local $1 i32)
+  (local $2 i32)
+  (local $3 i32)
+  (i32.store
+   (local.tee $0
+    (call $fimport$0
+     (i32.const 40)
+    )
+   )
+   (i32.const 0)
+  )
+  (local.set $1
+   (call $fimport$1
+    (local.get $0)
+    (i32.const 1)
+    (local.get $0)
+    (i32.const 4)
+   )
+  )
+  (local.set $2
+   (call $fimport$2)
+  )
+  (local.set $0
+   (i32.const 0)
+  )
+  (block $label$1
+   (block $label$2
+    (loop $label$3
+     (br_if $label$2
+      (local.get $0)
+     )
+     (i32.store offset=568
+      (i32.const 0)
+      (i32.const 0)
+     )
+     (call $fimport$4
+      (i32.const 1)
+      (local.get $0)
+      (i32.const 1)
+     )
+     (local.set $0
+      (i32.load offset=568
+       (i32.const 0)
+      )
+     )
+     (i32.store offset=568
+      (i32.const 0)
+      (i32.const 0)
+     )
+     (block $label$4
+      (br_if $label$4
+       (i32.eqz
+        (local.get $0)
+       )
+      )
+      (br_if $label$4
+       (i32.eqz
+        (local.tee $3
+         (i32.load offset=572
+          (i32.const 0)
+         )
+        )
+       )
+      )
+      (br_if $label$1
+       (i32.eqz
+        (call $fimport$5
+         (i32.load
+          (local.get $0)
+         )
+         (local.get $1)
+         (local.get $2)
+        )
+       )
+      )
+      (call $fimport$6
+       (local.get $3)
+      )
+     )
+     (local.set $0
+      (call $fimport$2)
+     )
+     (br $label$3)
+    )
+   )
+   (call $fimport$7
+    (local.get $1)
+   )
+   (return
+    (i32.const 0)
+   )
+  )
+  (call $fimport$8
+   (local.get $0)
+   (local.get $3)
+  )
+  (unreachable)
+ )
+ (func $2 (; 11 ;) (param $0 i32) (param $1 i32) (result i32)
+  (call $1)
+ )
+ ;; custom section "producers", size 112
+)
+