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
emscripten-core · Nov 11, 2020 · 50fc8c1 · 50fc8c1
1 parent 8091b98
commit 50fc8c1
Show file tree

Hide file tree

Showing 5 changed files with 142 additions and 66 deletions.
diff --git a/ChangeLog.md b/ChangeLog.md
@@ -48,6 +48,13 @@ Current Trunk
   input that perform all the normal post link steps such as finalizing and
   optimizing the wasm file and generating the JavaScript and/or html that will
   run it.
+- Added emulation support and a build time warning for calling Wasm function
+  pointers from JS library files via the old syntax
+        {{{ makeDynCall('sig') }}} (ptr, arg1, arg2);
+  that was broken on Aug 31st 2020 (Emscripten 2.0.2). A build warning will now
+  be emitted if the old signature is used. Convert to using the new signature
+        {{{ makeDynCall('sig', 'ptr') }}} (arg1, arg2);
+  instead.
 
 2.0.8: 10/24/2020
 -----------------
@@ -134,6 +141,30 @@ Current Trunk
 
 2.0.3: 09/10/2020
 -----------------
+- Breaking changes to calling Wasm function pointers from JavaScript:
+  1. It is no longer possible to directly call dynCall_sig(funcPtr, param) to
+    call a function pointer from JavaScript code. As a result, JavaScript code
+    outside all JS libraries (pre-js/post-js/EM_ASM/EM_JS/external JS code) can no
+    longer call a function pointer via static signature matching dynCall_sig, but
+    must instead use the dynamic binding function
+
+       dynCall(sig, ptr, args);
+
+    This carries a significant performance overhead. The function dynCall is not
+    available in -s MINIMAL_RUNTIME=1 builds.
+  2. old syntax for calling a Wasm function pointer from a JS library file used
+     to be
+
+      {{{ makeDynCall('sig') }}} (ptr, arg1, arg2);
+
+    This syntax will no longer work, and until Emscripten <2.0.9 causes
+    a runtime error TypeError: WebAssembly.Table.get(): Argument 0 must be
+    convertible to a valid number.
+
+    New syntax for calling Wasm function pointers from JS library files is
+
+      {{{ makeDynCall('sig', 'ptr') }}} (arg1, arg2);
+
 - The native optimizer and the corresponding config setting
   (`EMSCRIPTEN_NATIVE_OPTIMIZER`) have been removed (it was only relevant to
   asmjs/fastcomp backend).

diff --git a/src/parseTools.js b/src/parseTools.js
@@ -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) {
-  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;
-      }
+  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.');
+        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 + '`';
             }
-            var after = trimmed.substring(trimmed.indexOf(' '));
-            var truthy = !!eval(after);
-            showStack.push(truthy);
-          } else if (first === '#include') {
+          } else {
             if (showStack.indexOf(false) === -1) {
-              var filename = line.substr(line.indexOf(' ')+1);
-              if (filename.indexOf('"') === 0) {
-                filename = filename.substr(1, filename.length - 2);
+              // Never emit more than one empty line at a time.
+              if (emptyLine && !line) {
+                continue;
               }
-              var included = read(filename);
-              var result = preprocess(included, filename);
-              if (result) {
-                ret += "// include: " + filename + "\n";
-                ret += result;
-                ret += "// end include: " + filename + "\n";
+              ret += line + '\n';
+              if (!line) {
+                emptyLine = true;
+              } else {
+                emptyLine = false;
               }
             }
-          } 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 {
+        } else { // !inStyle
           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;
       }
-    } 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;
   }
-  assert(showStack.length == 0, 'preprocessing error in file '+ filenameHint + ', no matching #endif found (' + showStack.length + ' unmatched preprocessing directives on stack)');
-  return ret;
 }
 
 function removePointing(type, num) {
@@ -978,6 +985,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}) })`;
+    }
+  }
   if (USE_LEGACY_DYNCALLS) {
     return `getDynCaller("${sig}", ${funcPtr})`;
   } else {

diff --git a/tests/library_test_old_dyncall_format.js b/tests/library_test_old_dyncall_format.js
@@ -0,0 +1,5 @@
+mergeInto(LibraryManager.library, {
+	callFunc: function(func, param1, param2) {
+		{{{ makeDynCall('vii') }}} (func, param1, param2);
+	}
+});
diff --git a/tests/test_old_dyncall_format.c b/tests/test_old_dyncall_format.c
@@ -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);
+}
diff --git a/tests/test_other.py b/tests/test_other.py
@@ -9616,6 +9616,11 @@ def test_oformat(self):
     err = self.expect_fail([EMCC, path_from_root('tests', 'hello_world.c'), '--oformat=foo'])
     self.assertContained("error: invalid output format: `foo` (must be one of ['wasm', 'js', 'mjs', 'html', 'bare']", err)
 
+  # Tests that the old format of {{{ makeDynCall('sig') }}}(func, param1) works
+  def test_old_makeDynCall_syntax(self):
+    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
+    self.assertContained('syntax for makeDynCall has changed', err)
+
   def test_post_link(self):
     err = self.run_process([EMCC, path_from_root('tests', 'hello_world.c'), '--oformat=bare', '-o', 'bare.wasm'], stderr=PIPE).stderr
     self.assertContained('--oformat=base/--post-link are experimental and subject to change', err)