Skip to content

Commit 50fc8c1

Browse files
jujjuj
authored
Fix regression that broke calls to makeDynCall in JS library code. (#12732)
* Fix regression that broke calls to makeDynCall in JS library code. Old syntax is {{{ makeDynCall('sig') }}}(funcPtr, arg1, arg2);. New syntax is {{{ makeDynCall('sig', 'funcPtr') }}}(arg1, arg2). With this change, both old and new syntax work, with a compile time warning issued for old syntax to migrate to new one. Add ChangeLog entry about broken static dynCall invocation outside JS library files. * Address review
1 parent 8091b98 commit 50fc8c1

File tree

5 files changed

+142
-66
lines changed

5 files changed

+142
-66
lines changed

‎ChangeLog.md

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,13 @@ Current Trunk
4848
input that perform all the normal post link steps such as finalizing and
4949
optimizing the wasm file and generating the JavaScript and/or html that will
5050
run it.
51+
- Added emulation support and a build time warning for calling Wasm function
52+
pointers from JS library files via the old syntax
53+
{{{ makeDynCall('sig') }}} (ptr, arg1, arg2);
54+
that was broken on Aug 31st 2020 (Emscripten 2.0.2). A build warning will now
55+
be emitted if the old signature is used. Convert to using the new signature
56+
{{{ makeDynCall('sig', 'ptr') }}} (arg1, arg2);
57+
instead.
5158

5259
2.0.8: 10/24/2020
5360
-----------------
@@ -134,6 +141,30 @@ Current Trunk
134141

135142
2.0.3: 09/10/2020
136143
-----------------
144+
- Breaking changes to calling Wasm function pointers from JavaScript:
145+
1. It is no longer possible to directly call dynCall_sig(funcPtr, param) to
146+
call a function pointer from JavaScript code. As a result, JavaScript code
147+
outside all JS libraries (pre-js/post-js/EM_ASM/EM_JS/external JS code) can no
148+
longer call a function pointer via static signature matching dynCall_sig, but
149+
must instead use the dynamic binding function
150+
151+
dynCall(sig, ptr, args);
152+
153+
This carries a significant performance overhead. The function dynCall is not
154+
available in -s MINIMAL_RUNTIME=1 builds.
155+
2. old syntax for calling a Wasm function pointer from a JS library file used
156+
to be
157+
158+
{{{ makeDynCall('sig') }}} (ptr, arg1, arg2);
159+
160+
This syntax will no longer work, and until Emscripten <2.0.9 causes
161+
a runtime error TypeError: WebAssembly.Table.get(): Argument 0 must be
162+
convertible to a valid number.
163+
164+
New syntax for calling Wasm function pointers from JS library files is
165+
166+
{{{ makeDynCall('sig', 'ptr') }}} (arg1, arg2);
167+
137168
- The native optimizer and the corresponding config setting
138169
(`EMSCRIPTEN_NATIVE_OPTIMIZER`) have been removed (it was only relevant to
139170
asmjs/fastcomp backend).

‎src/parseTools.js

Lines changed: 88 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
// Various tools for parsing LLVM. Utilities of various sorts, that are
88
// specific to Emscripten (and hence not in utility.js).
99

10+
var currentlyParsedFilename = '';
11+
1012
// Does simple 'macro' substitution, using Django-like syntax,
1113
// {{{ code }}} will be replaced with |eval(code)|.
1214
// NOTE: Be careful with that ret check. If ret is |0|, |ret ? ret.toString() : ''| would result in ''!
@@ -29,86 +31,91 @@ function processMacros(text) {
2931
// Param filenameHint can be passed as a description to identify the file that is being processed, used
3032
// to locate errors for reporting and for html files to stop expansion between <style> and </style>.
3133
function preprocess(text, filenameHint) {
32-
var fileExt = (filenameHint) ? filenameHint.split('.').pop().toLowerCase() : "";
33-
var isHtml = (fileExt === 'html' || fileExt === 'htm') ? true : false;
34-
var inStyle = false;
35-
var lines = text.split('\n');
36-
var ret = '';
37-
var showStack = [];
38-
var emptyLine = false;
39-
for (var i = 0; i < lines.length; i++) {
40-
var line = lines[i];
41-
try {
42-
if (line[line.length-1] === '\r') {
43-
line = line.substr(0, line.length-1); // Windows will have '\r' left over from splitting over '\r\n'
44-
}
45-
if (isHtml && line.indexOf('<style') !== -1 && !inStyle) {
46-
inStyle = true;
47-
}
48-
if (isHtml && line.indexOf('</style') !== -1 && inStyle) {
49-
inStyle = false;
50-
}
34+
currentlyParsedFilename = filenameHint;
35+
try {
36+
var fileExt = (filenameHint) ? filenameHint.split('.').pop().toLowerCase() : "";
37+
var isHtml = (fileExt === 'html' || fileExt === 'htm') ? true : false;
38+
var inStyle = false;
39+
var lines = text.split('\n');
40+
var ret = '';
41+
var showStack = [];
42+
var emptyLine = false;
43+
for (var i = 0; i < lines.length; i++) {
44+
var line = lines[i];
45+
try {
46+
if (line[line.length-1] === '\r') {
47+
line = line.substr(0, line.length-1); // Windows will have '\r' left over from splitting over '\r\n'
48+
}
49+
if (isHtml && line.indexOf('<style') !== -1 && !inStyle) {
50+
inStyle = true;
51+
}
52+
if (isHtml && line.indexOf('</style') !== -1 && inStyle) {
53+
inStyle = false;
54+
}
5155

52-
if (!inStyle) {
53-
var trimmed = line.trim()
54-
if (trimmed[0] === '#') {
55-
var first = trimmed.split(' ', 1)[0]
56-
if (first == '#if' || first == '#ifdef') {
57-
if (first == '#ifdef') {
58-
warn('warning: use of #ifdef in js library. Use #if instead.');
56+
if (!inStyle) {
57+
var trimmed = line.trim()
58+
if (trimmed[0] === '#') {
59+
var first = trimmed.split(' ', 1)[0]
60+
if (first == '#if' || first == '#ifdef') {
61+
if (first == '#ifdef') {
62+
warn('warning: use of #ifdef in js library. Use #if instead.');
63+
}
64+
var after = trimmed.substring(trimmed.indexOf(' '));
65+
var truthy = !!eval(after);
66+
showStack.push(truthy);
67+
} else if (first === '#include') {
68+
if (showStack.indexOf(false) === -1) {
69+
var filename = line.substr(line.indexOf(' ')+1);
70+
if (filename.indexOf('"') === 0) {
71+
filename = filename.substr(1, filename.length - 2);
72+
}
73+
var included = read(filename);
74+
var result = preprocess(included, filename);
75+
if (result) {
76+
ret += "// include: " + filename + "\n";
77+
ret += result;
78+
ret += "// end include: " + filename + "\n";
79+
}
80+
}
81+
} else if (first === '#else') {
82+
assert(showStack.length > 0);
83+
showStack.push(!showStack.pop());
84+
} else if (first === '#endif') {
85+
assert(showStack.length > 0);
86+
showStack.pop();
87+
} else {
88+
throw "Unknown preprocessor directive on line " + i + ': `' + line + '`';
5989
}
60-
var after = trimmed.substring(trimmed.indexOf(' '));
61-
var truthy = !!eval(after);
62-
showStack.push(truthy);
63-
} else if (first === '#include') {
90+
} else {
6491
if (showStack.indexOf(false) === -1) {
65-
var filename = line.substr(line.indexOf(' ')+1);
66-
if (filename.indexOf('"') === 0) {
67-
filename = filename.substr(1, filename.length - 2);
92+
// Never emit more than one empty line at a time.
93+
if (emptyLine && !line) {
94+
continue;
6895
}
69-
var included = read(filename);
70-
var result = preprocess(included, filename);
71-
if (result) {
72-
ret += "// include: " + filename + "\n";
73-
ret += result;
74-
ret += "// end include: " + filename + "\n";
96+
ret += line + '\n';
97+
if (!line) {
98+
emptyLine = true;
99+
} else {
100+
emptyLine = false;
75101
}
76102
}
77-
} else if (first === '#else') {
78-
assert(showStack.length > 0);
79-
showStack.push(!showStack.pop());
80-
} else if (first === '#endif') {
81-
assert(showStack.length > 0);
82-
showStack.pop();
83-
} else {
84-
throw "Unknown preprocessor directive on line " + i + ': `' + line + '`';
85103
}
86-
} else {
104+
} else { // !inStyle
87105
if (showStack.indexOf(false) === -1) {
88-
// Never emit more than one empty line at a time.
89-
if (emptyLine && !line) {
90-
continue;
91-
}
92106
ret += line + '\n';
93-
if (!line) {
94-
emptyLine = true;
95-
} else {
96-
emptyLine = false;
97-
}
98107
}
99108
}
100-
} else { // !inStyle
101-
if (showStack.indexOf(false) === -1) {
102-
ret += line + '\n';
103-
}
109+
} catch(e) {
110+
printErr('parseTools.js preprocessor error in ' + filenameHint + ':' + (i+1) + ': \"' + line + '\"!');
111+
throw e;
104112
}
105-
} catch(e) {
106-
printErr('parseTools.js preprocessor error in ' + filenameHint + ':' + (i+1) + ': \"' + line + '\"!');
107-
throw e;
108113
}
114+
assert(showStack.length == 0, 'preprocessing error in file '+ filenameHint + ', no matching #endif found (' + showStack.length + ' unmatched preprocessing directives on stack)');
115+
return ret;
116+
} finally {
117+
currentlyParsedFilename = null;
109118
}
110-
assert(showStack.length == 0, 'preprocessing error in file '+ filenameHint + ', no matching #endif found (' + showStack.length + ' unmatched preprocessing directives on stack)');
111-
return ret;
112119
}
113120

114121
function removePointing(type, num) {
@@ -978,6 +985,21 @@ function asmFFICoercion(value, type) {
978985

979986
function makeDynCall(sig, funcPtr) {
980987
assert(sig.indexOf('j') == -1);
988+
if (funcPtr === undefined) {
989+
printErr(`warning: ${currentlyParsedFilename}: Legacy use of {{{ makeDynCall("${sig}") }}}(funcPtr, arg1, arg2, ...). Starting from Emscripten 2.0.2 (Aug 31st 2020), syntax for makeDynCall has changed. New syntax is {{{ makeDynCall("${sig}", "funcPtr") }}}(arg1, arg2, ...). Please update to new syntax.`);
990+
var ret = (sig[0] == 'v') ? 'return ' : '';
991+
var args = [];
992+
for(var i = 1; i < sig.length; ++i) {
993+
args.push(`a${i}`);
994+
}
995+
args = args.join(', ');
996+
997+
if (USE_LEGACY_DYNCALLS) {
998+
return `(function(cb, ${args}) { return getDynCaller("${sig}", cb)(${args}) })`;
999+
} else {
1000+
return `(function(cb, ${args}) { return wasmTable.get(cb)(${args}) })`;
1001+
}
1002+
}
9811003
if (USE_LEGACY_DYNCALLS) {
9821004
return `getDynCaller("${sig}", ${funcPtr})`;
9831005
} else {

‎tests/library_test_old_dyncall_format.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
mergeInto(LibraryManager.library, {
2+
callFunc: function(func, param1, param2) {
3+
{{{ makeDynCall('vii') }}} (func, param1, param2);
4+
}
5+
});

‎tests/test_old_dyncall_format.c

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
#include <stdio.h>
2+
3+
void foo(int param1, int param2)
4+
{
5+
if (param1 == 42 && param2 == 100) printf("Test passed!\n");
6+
}
7+
8+
extern void callFunc(void (*foo)(int, int), int param1, int param2);
9+
10+
int main()
11+
{
12+
callFunc(foo, 42, 100);
13+
}

‎tests/test_other.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9616,6 +9616,11 @@ def test_oformat(self):
96169616
err = self.expect_fail([EMCC, path_from_root('tests', 'hello_world.c'), '--oformat=foo'])
96179617
self.assertContained("error: invalid output format: `foo` (must be one of ['wasm', 'js', 'mjs', 'html', 'bare']", err)
96189618

9619+
# Tests that the old format of {{{ makeDynCall('sig') }}}(func, param1) works
9620+
def test_old_makeDynCall_syntax(self):
9621+
err = self.run_process([EMCC, path_from_root('tests', 'test_old_dyncall_format.c'), '--js-library', path_from_root('tests', 'library_test_old_dyncall_format.js')], stderr=PIPE).stderr
9622+
self.assertContained('syntax for makeDynCall has changed', err)
9623+
96199624
def test_post_link(self):
96209625
err = self.run_process([EMCC, path_from_root('tests', 'hello_world.c'), '--oformat=bare', '-o', 'bare.wasm'], stderr=PIPE).stderr
96219626
self.assertContained('--oformat=base/--post-link are experimental and subject to change', err)

0 commit comments

Comments
 (0)