Skip to content

Commit f62e171

Browse files
sbc100sbc100
authored
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 parent bd4cac9 commit f62e171

8 files changed

Lines changed: 462 additions & 72 deletions

File tree

‎scripts/test/generate_lld_tests.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ def files_with_extensions(path, extensions):
3030
yield file, ext
3131

3232

33-
def generate_wast_files(llvm_bin, emscripten_root):
34-
print('\n[ building wast files from C sources... ]\n')
33+
def generate_wat_files(llvm_bin, emscripten_root):
34+
print('\n[ building wat files from C sources... ]\n')
3535

3636
lld_path = os.path.join(shared.options.binaryen_test, 'lld')
3737
for src_file, ext in files_with_extensions(lld_path, ['.c', '.cpp']):
@@ -42,11 +42,11 @@ def generate_wast_files(llvm_bin, emscripten_root):
4242
obj_path = os.path.join(lld_path, obj_file)
4343

4444
wasm_file = src_file.replace(ext, '.wasm')
45-
wast_file = src_file.replace(ext, '.wast')
45+
wat_file = src_file.replace(ext, '.wat')
4646

4747
obj_path = os.path.join(lld_path, obj_file)
4848
wasm_path = os.path.join(lld_path, wasm_file)
49-
wast_path = os.path.join(lld_path, wast_file)
49+
wat_path = os.path.join(lld_path, wat_file)
5050
is_shared = 'shared' in src_file
5151

5252
compile_cmd = [
@@ -70,6 +70,10 @@ def generate_wast_files(llvm_bin, emscripten_root):
7070
'--export', '__data_end',
7171
'--global-base=568',
7272
]
73+
# We had a regression where this test only worked if debug names
74+
# were included.
75+
if 'longjmp' in src_file:
76+
link_cmd.append('--strip-debug')
7377
if is_shared:
7478
compile_cmd.append('-fPIC')
7579
compile_cmd.append('-fvisibility=default')
@@ -80,7 +84,7 @@ def generate_wast_files(llvm_bin, emscripten_root):
8084
try:
8185
support.run_command(compile_cmd)
8286
support.run_command(link_cmd)
83-
support.run_command(shared.WASM_DIS + [wasm_path, '-o', wast_path])
87+
support.run_command(shared.WASM_DIS + [wasm_path, '-o', wat_path])
8488
finally:
8589
# Don't need the .o or .wasm files, don't leave them around
8690
shared.delete_from_orbit(obj_path)
@@ -91,4 +95,4 @@ def generate_wast_files(llvm_bin, emscripten_root):
9195
if len(shared.options.positional_args) != 2:
9296
print('Usage: generate_lld_tests.py [llvm/bin/dir] [path/to/emscripten]')
9397
sys.exit(1)
94-
generate_wast_files(*shared.options.positional_args)
98+
generate_wat_files(*shared.options.positional_args)

‎scripts/test/shared.py‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ def parse_args(args):
8787

8888
options = parse_args(sys.argv[1:])
8989
requested = options.positional_args
90+
script_dir = os.path.dirname(os.path.abspath(__file__))
9091

9192
num_failures = 0
9293
warnings = []
@@ -123,8 +124,7 @@ def warn(text):
123124

124125
# Locate Binaryen source directory if not specified.
125126
if not options.binaryen_root:
126-
path_parts = os.path.abspath(__file__).split(os.path.sep)
127-
options.binaryen_root = os.path.sep.join(path_parts[:-3])
127+
options.binaryen_root = os.path.dirname(os.path.dirname(script_dir))
128128

129129
options.binaryen_test = os.path.join(options.binaryen_root, 'test')
130130

@@ -205,8 +205,7 @@ def wrap_with_valgrind(cmd):
205205

206206

207207
def in_binaryen(*args):
208-
__rootpath__ = os.path.abspath(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))
209-
return os.path.join(__rootpath__, *args)
208+
return os.path.join(options.binaryen_root, *args)
210209

211210

212211
os.environ['BINARYEN'] = in_binaryen()

‎src/wasm/wasm-emscripten.cpp‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -993,11 +993,11 @@ struct FixInvokeFunctionNamesWalker
993993
: public PostWalker<FixInvokeFunctionNamesWalker> {
994994
Module& wasm;
995995
std::map<Name, Name> importRenames;
996-
std::vector<Name> toRemove;
997-
std::set<Name> newImports;
996+
std::map<Name, Name> functionReplace;
998997
std::set<Signature> invokeSigs;
998+
ImportInfo imports;
999999

1000-
FixInvokeFunctionNamesWalker(Module& _wasm) : wasm(_wasm) {}
1000+
FixInvokeFunctionNamesWalker(Module& _wasm) : wasm(_wasm), imports(wasm) {}
10011001

10021002
// Converts invoke wrapper names generated by LLVM backend to real invoke
10031003
// wrapper names that are expected by JavaScript glue code.
@@ -1052,27 +1052,38 @@ struct FixInvokeFunctionNamesWalker
10521052
return;
10531053
}
10541054

1055-
assert(importRenames.count(curr->name) == 0);
1056-
BYN_TRACE("renaming: " << curr->name << " -> " << newname << "\n");
1057-
importRenames[curr->name] = newname;
1058-
// Either rename or remove the existing import
1059-
if (wasm.getFunctionOrNull(newname) || !newImports.insert(newname).second) {
1060-
toRemove.push_back(curr->name);
1055+
BYN_TRACE("renaming import: " << curr->module << "." << curr->base << " ("
1056+
<< curr->name << ") -> " << newname << "\n");
1057+
assert(importRenames.count(curr->base) == 0);
1058+
importRenames[curr->base] = newname;
1059+
// Either rename the import, or replace it with an existing one
1060+
Function* existingFunc = imports.getImportedFunction(curr->module, newname);
1061+
if (existingFunc) {
1062+
BYN_TRACE("replacing with an existing import: " << existingFunc->name
1063+
<< "\n");
1064+
functionReplace[curr->name] = existingFunc->name;
10611065
} else {
1066+
BYN_TRACE("renaming the import in place\n");
10621067
curr->base = newname;
1063-
curr->name = newname;
10641068
}
10651069
}
10661070

10671071
void visitModule(Module* curr) {
1068-
for (auto importName : toRemove) {
1069-
wasm.removeFunction(importName);
1072+
// For each replaced function first remove the function itself then
1073+
// rename all uses to the point to the new function.
1074+
for (auto& pair : functionReplace) {
1075+
BYN_TRACE("removeFunction " << pair.first << "\n");
1076+
wasm.removeFunction(pair.first);
10701077
}
1071-
ModuleUtils::renameFunctions(wasm, importRenames);
1072-
ImportInfo imports(wasm);
1078+
// Rename all uses of the removed functions
1079+
ModuleUtils::renameFunctions(wasm, functionReplace);
1080+
1081+
// For imports that for renamed, update any associated GOT.func imports.
10731082
for (auto& pair : importRenames) {
1074-
// Update any associated GOT.func import.
1083+
BYN_TRACE("looking for: GOT.func." << pair.first << "\n");
10751084
if (auto g = imports.getImportedGlobal("GOT.func", pair.first)) {
1085+
BYN_TRACE("renaming corresponding GOT entry: " << g->base << " -> "
1086+
<< pair.second << "\n");
10761087
g->base = pair.second;
10771088
}
10781089
}

‎test/lld/longjmp.c‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
typedef struct jmp_buf_buf {
2+
int thing;
3+
} jmp_buf;
4+
5+
void longjmp(jmp_buf env, int val);
6+
int setjmp(jmp_buf env);
7+
8+
int __THREW__;
9+
int __threwValue;
10+
11+
int main() {
12+
jmp_buf jmp;
13+
if (setjmp(jmp) == 0) {
14+
longjmp(jmp, 1);
15+
}
16+
return 0;
17+
}

‎test/lld/longjmp.wat‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
(module
2+
(type $i32_=>_none (func (param i32)))
3+
(type $i32_i32_=>_none (func (param i32 i32)))
4+
(type $none_=>_i32 (func (result i32)))
5+
(type $none_=>_none (func))
6+
(type $i32_i32_i32_=>_none (func (param i32 i32 i32)))
7+
(type $i32_=>_i32 (func (param i32) (result i32)))
8+
(type $i32_i32_=>_i32 (func (param i32 i32) (result i32)))
9+
(type $i32_i32_i32_=>_i32 (func (param i32 i32 i32) (result i32)))
10+
(type $i32_i32_i32_i32_=>_i32 (func (param i32 i32 i32 i32) (result i32)))
11+
(import "env" "malloc" (func $fimport$0 (param i32) (result i32)))
12+
(import "env" "saveSetjmp" (func $fimport$1 (param i32 i32 i32 i32) (result i32)))
13+
(import "env" "getTempRet0" (func $fimport$2 (result i32)))
14+
(import "env" "emscripten_longjmp_jmpbuf" (func $fimport$3 (param i32 i32)))
15+
(import "env" "__invoke_void_i32_i32" (func $fimport$4 (param i32 i32 i32)))
16+
(import "env" "testSetjmp" (func $fimport$5 (param i32 i32 i32) (result i32)))
17+
(import "env" "setTempRet0" (func $fimport$6 (param i32)))
18+
(import "env" "free" (func $fimport$7 (param i32)))
19+
(import "env" "emscripten_longjmp" (func $fimport$8 (param i32 i32)))
20+
(memory $0 2)
21+
(table $0 2 2 funcref)
22+
(elem (i32.const 1) $fimport$3)
23+
(global $global$0 (mut i32) (i32.const 66112))
24+
(global $global$1 i32 (i32.const 576))
25+
(export "memory" (memory $0))
26+
(export "__wasm_call_ctors" (func $0))
27+
(export "main" (func $2))
28+
(export "__data_end" (global $global$1))
29+
(func $0 (; 9 ;)
30+
)
31+
(func $1 (; 10 ;) (result i32)
32+
(local $0 i32)
33+
(local $1 i32)
34+
(local $2 i32)
35+
(local $3 i32)
36+
(i32.store
37+
(local.tee $0
38+
(call $fimport$0
39+
(i32.const 40)
40+
)
41+
)
42+
(i32.const 0)
43+
)
44+
(local.set $1
45+
(call $fimport$1
46+
(local.get $0)
47+
(i32.const 1)
48+
(local.get $0)
49+
(i32.const 4)
50+
)
51+
)
52+
(local.set $2
53+
(call $fimport$2)
54+
)
55+
(local.set $0
56+
(i32.const 0)
57+
)
58+
(block $label$1
59+
(block $label$2
60+
(loop $label$3
61+
(br_if $label$2
62+
(local.get $0)
63+
)
64+
(i32.store offset=568
65+
(i32.const 0)
66+
(i32.const 0)
67+
)
68+
(call $fimport$4
69+
(i32.const 1)
70+
(local.get $0)
71+
(i32.const 1)
72+
)
73+
(local.set $0
74+
(i32.load offset=568
75+
(i32.const 0)
76+
)
77+
)
78+
(i32.store offset=568
79+
(i32.const 0)
80+
(i32.const 0)
81+
)
82+
(block $label$4
83+
(br_if $label$4
84+
(i32.eqz
85+
(local.get $0)
86+
)
87+
)
88+
(br_if $label$4
89+
(i32.eqz
90+
(local.tee $3
91+
(i32.load offset=572
92+
(i32.const 0)
93+
)
94+
)
95+
)
96+
)
97+
(br_if $label$1
98+
(i32.eqz
99+
(call $fimport$5
100+
(i32.load
101+
(local.get $0)
102+
)
103+
(local.get $1)
104+
(local.get $2)
105+
)
106+
)
107+
)
108+
(call $fimport$6
109+
(local.get $3)
110+
)
111+
)
112+
(local.set $0
113+
(call $fimport$2)
114+
)
115+
(br $label$3)
116+
)
117+
)
118+
(call $fimport$7
119+
(local.get $1)
120+
)
121+
(return
122+
(i32.const 0)
123+
)
124+
)
125+
(call $fimport$8
126+
(local.get $0)
127+
(local.get $3)
128+
)
129+
(unreachable)
130+
)
131+
(func $2 (; 11 ;) (param $0 i32) (param $1 i32) (result i32)
132+
(call $1)
133+
)
134+
;; custom section "producers", size 112
135+
)
136+

0 commit comments

Comments
 (0)