Skip to content

Commit f0a2e2c

Browse files
sbc100sbc100
authored
Fix renaming in FixInvokeFunctionNamesWalker (#2513)
This fixes emscripten-core/emscripten#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.
1 parent fad92ea commit f0a2e2c

5 files changed

Lines changed: 73 additions & 65 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 'shared_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

@@ -207,8 +207,7 @@ def wrap_with_valgrind(cmd):
207207

208208

209209
def in_binaryen(*args):
210-
__rootpath__ = os.path.abspath(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))
211-
return os.path.join(__rootpath__, *args)
210+
return os.path.join(options.binaryen_root, *args)
212211

213212

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

‎src/wasm/wasm-emscripten.cpp‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -996,8 +996,9 @@ struct FixInvokeFunctionNamesWalker
996996
std::vector<Name> toRemove;
997997
std::set<Name> newImports;
998998
std::set<Signature> invokeSigs;
999+
ImportInfo imports;
9991000

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

10021003
// Converts invoke wrapper names generated by LLVM backend to real invoke
10031004
// wrapper names that are expected by JavaScript glue code.
@@ -1052,15 +1053,18 @@ struct FixInvokeFunctionNamesWalker
10521053
return;
10531054
}
10541055

1055-
assert(importRenames.count(curr->name) == 0);
1056-
BYN_TRACE("renaming: " << curr->name << " -> " << newname << "\n");
1057-
importRenames[curr->name] = newname;
1056+
BYN_TRACE("renaming import: " << curr->module << "." << curr->base << " ("
1057+
<< curr->name << ") -> " << newname << "\n");
1058+
assert(importRenames.count(curr->base) == 0);
1059+
importRenames[curr->base] = newname;
10581060
// Either rename or remove the existing import
1059-
if (wasm.getFunctionOrNull(newname) || !newImports.insert(newname).second) {
1061+
if (imports.getImportedFunction(curr->module, newname) ||
1062+
!newImports.insert(newname).second) {
1063+
BYN_TRACE("using existing import\n");
10601064
toRemove.push_back(curr->name);
10611065
} else {
1066+
BYN_TRACE("renaming import\n");
10621067
curr->base = newname;
1063-
curr->name = newname;
10641068
}
10651069
}
10661070

@@ -1069,10 +1073,11 @@ struct FixInvokeFunctionNamesWalker
10691073
wasm.removeFunction(importName);
10701074
}
10711075
ModuleUtils::renameFunctions(wasm, importRenames);
1072-
ImportInfo imports(wasm);
1076+
// Update any associated GOT.func imports.
10731077
for (auto& pair : importRenames) {
1074-
// Update any associated GOT.func import.
10751078
if (auto g = imports.getImportedGlobal("GOT.func", pair.first)) {
1079+
BYN_TRACE("renaming corresponding GOT entry: " << g->base << " -> "
1080+
<< pair.second << "\n");
10761081
g->base = pair.second;
10771082
}
10781083
}

‎test/lld/shared_longjmp.wat‎

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,49 +15,49 @@
1515
(import "GOT.mem" "__THREW__" (global $gimport$13 (mut i32)))
1616
(import "GOT.func" "emscripten_longjmp_jmpbuf" (global $gimport$14 (mut i32)))
1717
(import "GOT.mem" "__threwValue" (global $gimport$15 (mut i32)))
18-
(import "env" "malloc" (func $malloc (param i32) (result i32)))
19-
(import "env" "saveSetjmp" (func $saveSetjmp (param i32 i32 i32 i32) (result i32)))
20-
(import "env" "getTempRet0" (func $getTempRet0 (result i32)))
21-
(import "env" "emscripten_longjmp_jmpbuf" (func $emscripten_longjmp_jmpbuf (param i32 i32)))
22-
(import "env" "__invoke_void_i32_i32" (func $__invoke_void_i32_i32 (param i32 i32 i32)))
23-
(import "env" "testSetjmp" (func $testSetjmp (param i32 i32 i32) (result i32)))
24-
(import "env" "setTempRet0" (func $setTempRet0 (param i32)))
25-
(import "env" "free" (func $free (param i32)))
26-
(import "env" "emscripten_longjmp" (func $emscripten_longjmp (param i32 i32)))
18+
(import "env" "malloc" (func $fimport$4 (param i32) (result i32)))
19+
(import "env" "saveSetjmp" (func $fimport$5 (param i32 i32 i32 i32) (result i32)))
20+
(import "env" "getTempRet0" (func $fimport$6 (result i32)))
21+
(import "env" "emscripten_longjmp_jmpbuf" (func $fimport$7 (param i32 i32)))
22+
(import "env" "__invoke_void_i32_i32" (func $fimport$8 (param i32 i32 i32)))
23+
(import "env" "testSetjmp" (func $fimport$9 (param i32 i32 i32) (result i32)))
24+
(import "env" "setTempRet0" (func $fimport$10 (param i32)))
25+
(import "env" "free" (func $fimport$11 (param i32)))
26+
(import "env" "emscripten_longjmp" (func $fimport$12 (param i32 i32)))
2727
(global $global$0 i32 (i32.const 0))
2828
(global $global$1 i32 (i32.const 4))
29-
(export "__wasm_call_ctors" (func $__wasm_call_ctors))
30-
(export "_start" (func $_start))
29+
(export "__wasm_call_ctors" (func $0))
30+
(export "_start" (func $2))
3131
(export "__THREW__" (global $global$0))
3232
(export "__threwValue" (global $global$1))
33-
(func $__wasm_call_ctors (; 9 ;) (type $7)
34-
(call $__wasm_apply_relocs)
33+
(func $0 (; 9 ;) (type $7)
34+
(call $1)
3535
)
36-
(func $__wasm_apply_relocs (; 10 ;) (type $7)
36+
(func $1 (; 10 ;) (type $7)
3737
)
38-
(func $_start (; 11 ;) (type $7)
38+
(func $2 (; 11 ;) (type $7)
3939
(local $0 i32)
4040
(local $1 i32)
4141
(local $2 i32)
4242
(local $3 i32)
4343
(i32.store
4444
(local.tee $0
45-
(call $malloc
45+
(call $fimport$4
4646
(i32.const 40)
4747
)
4848
)
4949
(i32.const 0)
5050
)
5151
(local.set $1
52-
(call $saveSetjmp
52+
(call $fimport$5
5353
(local.get $0)
5454
(i32.const 1)
5555
(local.get $0)
5656
(i32.const 4)
5757
)
5858
)
5959
(local.set $2
60-
(call $getTempRet0)
60+
(call $fimport$6)
6161
)
6262
(local.set $0
6363
(i32.const 0)
@@ -74,7 +74,7 @@
7474
)
7575
(i32.const 0)
7676
)
77-
(call $__invoke_void_i32_i32
77+
(call $fimport$8
7878
(global.get $gimport$14)
7979
(local.get $0)
8080
(i32.const 1)
@@ -108,7 +108,7 @@
108108
)
109109
(br_if $label$1
110110
(i32.eqz
111-
(call $testSetjmp
111+
(call $fimport$9
112112
(i32.load
113113
(local.get $3)
114114
)
@@ -117,22 +117,22 @@
117117
)
118118
)
119119
)
120-
(call $setTempRet0
120+
(call $fimport$10
121121
(local.get $0)
122122
)
123123
)
124124
(local.set $0
125-
(call $getTempRet0)
125+
(call $fimport$6)
126126
)
127127
(br $label$3)
128128
)
129129
)
130-
(call $free
130+
(call $fimport$11
131131
(local.get $1)
132132
)
133133
(return)
134134
)
135-
(call $emscripten_longjmp
135+
(call $fimport$12
136136
(local.get $3)
137137
(local.get $0)
138138
)

‎test/lld/shared_longjmp.wat.out‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,14 @@
1212
(import "env" "table" (table $0 0 funcref))
1313
(import "env" "__memory_base" (global $gimport$2 i32))
1414
(import "env" "__table_base" (global $gimport$3 i32))
15-
(import "env" "malloc" (func $malloc (param i32) (result i32)))
16-
(import "env" "saveSetjmp" (func $saveSetjmp (param i32 i32 i32 i32) (result i32)))
17-
(import "env" "getTempRet0" (func $getTempRet0 (result i32)))
18-
(import "env" "invoke_vii" (func $invoke_vii (param i32 i32 i32)))
19-
(import "env" "testSetjmp" (func $testSetjmp (param i32 i32 i32) (result i32)))
20-
(import "env" "setTempRet0" (func $setTempRet0 (param i32)))
21-
(import "env" "free" (func $free (param i32)))
22-
(import "env" "emscripten_longjmp" (func $emscripten_longjmp (param i32 i32)))
15+
(import "env" "malloc" (func $fimport$4 (param i32) (result i32)))
16+
(import "env" "saveSetjmp" (func $fimport$5 (param i32 i32 i32 i32) (result i32)))
17+
(import "env" "getTempRet0" (func $fimport$6 (result i32)))
18+
(import "env" "invoke_vii" (func $fimport$8 (param i32 i32 i32)))
19+
(import "env" "testSetjmp" (func $fimport$9 (param i32 i32 i32) (result i32)))
20+
(import "env" "setTempRet0" (func $fimport$10 (param i32)))
21+
(import "env" "free" (func $fimport$11 (param i32)))
22+
(import "env" "emscripten_longjmp" (func $fimport$12 (param i32 i32)))
2323
(import "env" "g$__THREW__" (func $g$__THREW__ (result i32)))
2424
(import "env" "g$__threwValue" (func $g$__threwValue (result i32)))
2525
(import "env" "fp$emscripten_longjmp$vii" (func $fp$emscripten_longjmp$vii (result i32)))
@@ -28,40 +28,40 @@
2828
(global $gimport$15 (mut i32) (i32.const 0))
2929
(global $global$0 i32 (i32.const 0))
3030
(global $global$1 i32 (i32.const 4))
31-
(export "_start" (func $_start))
31+
(export "_start" (func $2))
3232
(export "__THREW__" (global $global$0))
3333
(export "__threwValue" (global $global$1))
3434
(export "dynCall_vii" (func $dynCall_vii))
3535
(export "__post_instantiate" (func $__post_instantiate))
36-
(func $__wasm_call_ctors (; 11 ;)
37-
(call $__wasm_apply_relocs)
36+
(func $0 (; 11 ;)
37+
(call $1)
3838
)
39-
(func $__wasm_apply_relocs (; 12 ;)
39+
(func $1 (; 12 ;)
4040
(nop)
4141
)
42-
(func $_start (; 13 ;)
42+
(func $2 (; 13 ;)
4343
(local $0 i32)
4444
(local $1 i32)
4545
(local $2 i32)
4646
(local $3 i32)
4747
(i32.store
4848
(local.tee $0
49-
(call $malloc
49+
(call $fimport$4
5050
(i32.const 40)
5151
)
5252
)
5353
(i32.const 0)
5454
)
5555
(local.set $1
56-
(call $saveSetjmp
56+
(call $fimport$5
5757
(local.get $0)
5858
(i32.const 1)
5959
(local.get $0)
6060
(i32.const 4)
6161
)
6262
)
6363
(local.set $2
64-
(call $getTempRet0)
64+
(call $fimport$6)
6565
)
6666
(local.set $0
6767
(i32.const 0)
@@ -78,7 +78,7 @@
7878
)
7979
(i32.const 0)
8080
)
81-
(call $invoke_vii
81+
(call $fimport$8
8282
(global.get $gimport$14)
8383
(local.get $0)
8484
(i32.const 1)
@@ -112,7 +112,7 @@
112112
)
113113
(br_if $label$1
114114
(i32.eqz
115-
(call $testSetjmp
115+
(call $fimport$9
116116
(i32.load
117117
(local.get $3)
118118
)
@@ -121,22 +121,22 @@
121121
)
122122
)
123123
)
124-
(call $setTempRet0
124+
(call $fimport$10
125125
(local.get $0)
126126
)
127127
)
128128
(local.set $0
129-
(call $getTempRet0)
129+
(call $fimport$6)
130130
)
131131
(br $label$3)
132132
)
133133
)
134-
(call $free
134+
(call $fimport$11
135135
(local.get $1)
136136
)
137137
(return)
138138
)
139-
(call $emscripten_longjmp
139+
(call $fimport$12
140140
(local.get $3)
141141
(local.get $0)
142142
)
@@ -151,7 +151,7 @@
151151
)
152152
(func $__post_instantiate (; 15 ;)
153153
(call $__assign_got_enties)
154-
(call $__wasm_call_ctors)
154+
(call $0)
155155
)
156156
(func $__assign_got_enties (; 16 ;)
157157
(global.set $gimport$13

0 commit comments

Comments
 (0)