Fix regression that broke calls to makeDynCall in JS library code. #12732
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ | |
| // Various tools for parsing LLVM. Utilities of various sorts, that are | ||
| // specific to Emscripten (and hence not in utility.js). | ||
|
|
||
| var currentlyParsedFilename = ''; | ||
|
|
||
| // Does simple 'macro' substitution, using Django-like syntax, | ||
| // {{{ code }}} will be replaced with |eval(code)|. | ||
| // NOTE: Be careful with that ret check. If ret is |0|, |ret ? ret.toString() : ''| would result in ''! | ||
|
|
@@ -29,86 +31,91 @@ function processMacros(text) { | |
| // Param filenameHint can be passed as a description to identify the file that is being processed, used | ||
| // to locate errors for reporting and for html files to stop expansion between <style> and </style>. | ||
| function preprocess(text, filenameHint) { | ||
| currentlyParsedFilename = filenameHint; | ||
| try { | ||
| var fileExt = (filenameHint) ? filenameHint.split('.').pop().toLowerCase() : ""; | ||
| var isHtml = (fileExt === 'html' || fileExt === 'htm') ? true : false; | ||
| var inStyle = false; | ||
| var lines = text.split('\n'); | ||
| var ret = ''; | ||
| var showStack = []; | ||
| var emptyLine = false; | ||
| for (var i = 0; i < lines.length; i++) { | ||
| var line = lines[i]; | ||
| try { | ||
| if (line[line.length-1] === '\r') { | ||
| line = line.substr(0, line.length-1); // Windows will have '\r' left over from splitting over '\r\n' | ||
| } | ||
| if (isHtml && line.indexOf('<style') !== -1 && !inStyle) { | ||
| inStyle = true; | ||
| } | ||
| if (isHtml && line.indexOf('</style') !== -1 && inStyle) { | ||
| inStyle = false; | ||
| } | ||
|
|
||
| if (!inStyle) { | ||
| var trimmed = line.trim() | ||
| if (trimmed[0] === '#') { | ||
| var first = trimmed.split(' ', 1)[0] | ||
| if (first == '#if' || first == '#ifdef') { | ||
| if (first == '#ifdef') { | ||
| warn('warning: use of #ifdef in js library. Use #if instead.'); | ||
| } | ||
| var after = trimmed.substring(trimmed.indexOf(' ')); | ||
| var truthy = !!eval(after); | ||
| showStack.push(truthy); | ||
| } else if (first === '#include') { | ||
| if (showStack.indexOf(false) === -1) { | ||
| var filename = line.substr(line.indexOf(' ')+1); | ||
| if (filename.indexOf('"') === 0) { | ||
| filename = filename.substr(1, filename.length - 2); | ||
| } | ||
| var included = read(filename); | ||
| var result = preprocess(included, filename); | ||
| if (result) { | ||
| ret += "// include: " + filename + "\n"; | ||
| ret += result; | ||
| ret += "// end include: " + filename + "\n"; | ||
| } | ||
| } | ||
| } else if (first === '#else') { | ||
| assert(showStack.length > 0); | ||
| showStack.push(!showStack.pop()); | ||
| } else if (first === '#endif') { | ||
| assert(showStack.length > 0); | ||
| showStack.pop(); | ||
| } else { | ||
| throw "Unknown preprocessor directive on line " + i + ': `' + line + '`'; | ||
| } | ||
| } else { | ||
| if (showStack.indexOf(false) === -1) { | ||
| // Never emit more than one empty line at a time. | ||
| if (emptyLine && !line) { | ||
| continue; | ||
| } | ||
| ret += line + '\n'; | ||
| if (!line) { | ||
| emptyLine = true; | ||
| } else { | ||
| emptyLine = false; | ||
| } | ||
| } | ||
| } | ||
| } else { // !inStyle | ||
| if (showStack.indexOf(false) === -1) { | ||
| ret += line + '\n'; | ||
| } | ||
| } | ||
| } catch(e) { | ||
| printErr('parseTools.js preprocessor error in ' + filenameHint + ':' + (i+1) + ': \"' + line + '\"!'); | ||
| throw e; | ||
| } | ||
| } | ||
| assert(showStack.length == 0, 'preprocessing error in file '+ filenameHint + ', no matching #endif found (' + showStack.length + ' unmatched preprocessing directives on stack)'); | ||
| return ret; | ||
| } finally { | ||
| currentlyParsedFilename = null; | ||
| } | ||
| } | ||
|
|
||
| function removePointing(type, num) { | ||
|
|
@@ -985,6 +992,21 @@ function asmFFICoercion(value, type) { | |
|
|
||
| function makeDynCall(sig, funcPtr) { | ||
| assert(sig.indexOf('j') == -1); | ||
| if (funcPtr === undefined) { | ||
| 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.`); | ||
| var ret = (sig[0] == 'v') ? 'return ' : ''; | ||
| var args = []; | ||
| for(var i = 1; i < sig.length; ++i) { | ||
| args.push(`a${i}`); | ||
| } | ||
| args = args.join(', '); | ||
|
|
||
| if (USE_LEGACY_DYNCALLS) { | ||
| return `(function(cb, ${args}) { return getDynCaller("${sig}", cb)(${args}) })`; | ||
| } else { | ||
| return `(function(cb, ${args}) { return wasmTable.get(cb)(${args}) })`; | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. I love the clear error message, but actually handling the old form feels excessive to me. I suggest erroring the build, to force people to update? It's an internal API, and one that isn't hard to update for (with the nice new error message here). There was a problem hiding this comment. I am surprised to see you say that this is an internal API. If this was an internal API (only used in Emscripten system library_*.js files), then indeed this would be excessive and I would not have minded too much about this, but would just fix our own library JS code in Unity and move on. However, Also we should not have core syntax in library_*.js that is not supposed to be used by end users. That is because most of the library_*.js files that people author are monkeyed off of existing src/library_*.js files - that is what we write to the GitHub issue tracker so many times in the past when people ask how to achieve something ("take a look at an example in src/library_webgl.js" etc). If Historically, according to my understanding, there have been three ways to invoke a function pointer from JS code:
PR #12059 regressed all of the syntaxes 1., 2. and 3. above (#12733). This PR fixes the regression (2). I do not know how to fix (1) at the moment, that is blocking us to be able to update Unity to a newer compiler version. The regression of (3) is that This regression (3) is minor for Unity's use, since we can paper over that in the build system easily, so not sweating too much about it (but it is still a breaking change that was not mentioned in the ChangeLog for Emscripten 2.0.2). Chatting with @sbc100 on #12733 I believe there was a misassumption that (3) would be the "one syntax to rule them all". Historically I moved us away from the dynamic dispatch dynCall in Jan 2017 because of its large impact observed on real-world performance (#4894). The benchmark in #12733 (comment) verifies that the performance landscape is still the same today. We should not be going back to that. Unity Asset Store has hundreds of packages that advertise WebGL support, and we have thousands to tens of thousands of users that develop their own WebGL project; of all of those an unknown number of packages and game projects WebGL packages with library_*.js files to interop with Unity C#, C/C++ and JS. We cannot break any of them when updating, because it may not even be up to the project developer to be able to fix up the library_*.js issue, but they must receive an update from the Asset Store package author. If we errored out here in this message, that would mean that possibly thousands of users would be blocked from developing their game project until plugin authors provided a fix. That is not possible for us. Because of these reasons is why I am adding a very explicit warning about this that will prompt developers who see the warning to look at their own library_*.js files, and nudge their plugin vendors to fix up theirs. See also the comment #12733 (comment). There was a problem hiding this comment. I see, thanks @juj, I didn't realize this was used in user code so much. I'd assumed that low-level handling indirect calls is extremely rare (like it is in the emscripten JS libraries). Optimally user code would use something that does the low-level handling for it, but that's not the situation. Given that, sounds good to support the old syntax. |
||
| if (USE_LEGACY_DYNCALLS) { | ||
| return `getDynCaller("${sig}", ${funcPtr})`; | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| mergeInto(LibraryManager.library, { | ||
| callFunc: function(func, param1, param2) { | ||
| {{{ makeDynCall('vii') }}} (func, param1, param2); | ||
| } | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #include <stdio.h> | ||
|
|
||
| void foo(int param1, int param2) | ||
| { | ||
| if (param1 == 42 && param2 == 100) printf("Test passed!\n"); | ||
| } | ||
|
|
||
| extern void callFunc(void (*foo)(int, int), int param1, int param2); | ||
|
|
||
| int main() | ||
| { | ||
| callFunc(foo, 42, 100); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.