embind: Fix method pointers for AOT JS generation.

Previously, AOT JS glue code for method pointers used the address of the method pointer as a unique ID to match JS glue with the binding. However, sometimes static initializers are run in different orders when running the AOT generation code versus when the code is run normally. This caused the method pointer address to be different and the binding didn't match up. Instead of using the function pointer or the method pointer address we now use a generated signature for the invoker to match up the glue code. This works for both types of pointers and also has the advantage that many of the glue code functions are nearly identical and can be reusued (closure arguments make them behave differently). Fixes emscripten-core#20994
brendandahl · Jan 25, 2024 · 3741aad · 3741aad
1 parent 3958a80
commit 3741aad
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 19 deletions.
diff --git a/src/embind/embind.js b/src/embind/embind.js
@@ -781,6 +781,7 @@ var LibraryEmbind = {
 #endif
 #if EMBIND_AOT
     '$InvokerFunctions',
+    '$createJsInvokerSignature',
 #endif
 #if ASYNCIFY
     '$Asyncify',
@@ -884,7 +885,7 @@ var LibraryEmbind = {
 #else
   // Builld the arguments that will be passed into the closure around the invoker
   // function.
-  var closureArgs = [throwBindingError, cppInvokerFunc, cppTargetFunc, runDestructors, argTypes[0], argTypes[1]];
+  var closureArgs = [humanName, throwBindingError, cppInvokerFunc, cppTargetFunc, runDestructors, argTypes[0], argTypes[1]];
 #if EMSCRIPTEN_TRACING
   closureArgs.push(Module);
 #endif
@@ -903,9 +904,10 @@ var LibraryEmbind = {
   }
 
 #if EMBIND_AOT
-  var invokerFn = InvokerFunctions[cppTargetFunc].apply(null, closureArgs);
+  var signature = createJsInvokerSignature(argTypes, isClassMethodFunc, returns, isAsync);
+  var invokerFn = InvokerFunctions[signature].apply(null, closureArgs);
 #else
-  let [args, invokerFnBody] = createJsInvoker(humanName, argTypes, isClassMethodFunc, returns, isAsync);
+  let [args, invokerFnBody] = createJsInvoker(argTypes, isClassMethodFunc, returns, isAsync);
   args.push(invokerFnBody);
   var invokerFn = newFunc(Function, args).apply(null, closureArgs);
 #endif

diff --git a/src/embind/embind_gen.js b/src/embind/embind_gen.js
@@ -40,7 +40,7 @@ var LibraryEmbind = {
       this.destructorType = 'none'; // Same as emval.
     }
   },
-  $FunctionDefinition__deps: ['$createJsInvoker'],
+  $FunctionDefinition__deps: ['$createJsInvoker', '$createJsInvokerSignature'],
   $FunctionDefinition: class {
     constructor(name, returnType, argumentTypes, functionIndex, thisType = null, isAsync = false) {
       this.name = name;
@@ -95,7 +95,7 @@ var LibraryEmbind = {
       return ret;
     }
 
-    printJs(out) {
+    printJs(emitted, out) {
       const argTypes = [this.convertToEmbindType(this.returnType)];
       if (this.thisType) {
         argTypes.push(this.convertToEmbindType(this.thisType));
@@ -105,10 +105,15 @@ var LibraryEmbind = {
       for (const argType of this.argumentTypes) {
         argTypes.push(this.convertToEmbindType(argType.type));
       }
-      let [args, body] = createJsInvoker(this.name, argTypes, !!this.thisType, this.returnType.name !== 'void', this.isAsync);
+      const signature = createJsInvokerSignature(argTypes, !!this.thisType, this.returnType.name !== 'void', this.isAsync)
+      if (emitted[signature]) {
+        return;
+      }
+      emitted[signature] = true;
+      let [args, body] = createJsInvoker(argTypes, !!this.thisType, this.returnType.name !== 'void', this.isAsync);
       out.push(
         // The ${""} is hack to workaround the preprocessor replacing "function".
-        `'${this.functionIndex}': f${""}unction(${args.join(',')}) {\n${body}},`
+        `'${signature}': f${""}unction(${args.join(',')}) {\n${body}},`
       );
     }
   },
@@ -182,24 +187,24 @@ var LibraryEmbind = {
       out.push('};\n');
     }
 
-    printJs(out) {
+    printJs(emitted, out) {
       out.push(`// class ${this.name}\n`);
       if (this.constructors.length) {
         out.push(`// constructors\n`);
         for (const construct of this.constructors) {
-          construct.printJs(out);
+          construct.printJs(emitted, out);
         }
       }
       if (this.staticMethods.length) {
         out.push(`// static methods\n`);
         for (const method of this.staticMethods) {
-          method.printJs(out);
+          method.printJs(emitted, out);
         }
       }
       if (this.methods.length) {
         out.push(`// methods\n`);
         for (const method of this.methods) {
-          method.printJs(out);
+          method.printJs(emitted, out);
         }
       }
       out.push('\n');
@@ -377,11 +382,12 @@ var LibraryEmbind = {
 
     print() {
       const out = ['{\n'];
+      const emitted = {};
       for (const def of this.definitions) {
         if (!def.printJs) {
           continue;
         }
-        def.printJs(out);
+        def.printJs(emitted, out);
       }
       out.push('}')
       console.log(out.join(''));

diff --git a/src/embind/embind_shared.js b/src/embind/embind_shared.js
@@ -179,8 +179,33 @@ var LibraryEmbindShared = {
     return false;
   },
 
+  // Many of the JS invoker functions are generic and can be reused for multiple
+  // function bindings. This function needs to match createJsInvoker and create
+  // a unique signature for any inputs that will create different invoker
+  // function outputs.
+  $createJsInvokerSignature(argTypes, isClassMethodFunc, returns, isAsync) {
+    const signature = [
+      isClassMethodFunc ? 't' : 'f',
+      returns ? 't' : 'f',
+      isAsync ? 't' : 'f'
+    ];
+    for (var i = isClassMethodFunc ? 1 : 2; i < argTypes.length; ++i) {
+      const arg = argTypes[i];
+      let destructorSig = '';
+      if (arg.destructorFunction === undefined) {
+        destructorSig = 'u';
+      } else if (arg.destructorFunction === null) {
+        destructorSig = 'n';
+      } else {
+        destructorSig = 't';
+      }
+      signature.push(destructorSig);
+    }
+    return signature.join('');
+  },
+
   $createJsInvoker__deps: ['$usesDestructorStack'],
-  $createJsInvoker(humanName, argTypes, isClassMethodFunc, returns, isAsync) {
+  $createJsInvoker(argTypes, isClassMethodFunc, returns, isAsync) {
     var needsDestructorStack = usesDestructorStack(argTypes);
     var argCount = argTypes.length;
     var argsList = "";
@@ -193,19 +218,19 @@ var LibraryEmbindShared = {
     var invokerFnBody = `
       return function (${argsList}) {
       if (arguments.length !== ${argCount - 2}) {
-        throwBindingError('function ${humanName} called with ' + arguments.length + ' arguments, expected ${argCount - 2}');
+        throwBindingError('function ' + humanName + ' called with ' + arguments.length + ' arguments, expected ${argCount - 2}');
       }`;
 
 #if EMSCRIPTEN_TRACING
-    invokerFnBody += `Module.emscripten_trace_enter_context('embind::${humanName}');\n`;
+    invokerFnBody += `Module.emscripten_trace_enter_context('embind::' + humanName );\n`;
 #endif
 
     if (needsDestructorStack) {
       invokerFnBody += "var destructors = [];\n";
     }
 
     var dtorStack = needsDestructorStack ? "destructors" : "null";
-    var args1 = ["throwBindingError", "invoker", "fn", "runDestructors", "retType", "classParam"];
+    var args1 = ["humanName", "throwBindingError", "invoker", "fn", "runDestructors", "retType", "classParam"];
 
 #if EMSCRIPTEN_TRACING
     args1.push("Module");
@@ -216,7 +241,7 @@ var LibraryEmbindShared = {
     }
 
     for (var i = 0; i < argCount - 2; ++i) {
-      invokerFnBody += "var arg"+i+"Wired = argType"+i+"['toWireType']("+dtorStack+", arg"+i+"); // "+argTypes[i+2].name+"\n";
+      invokerFnBody += "var arg"+i+"Wired = argType"+i+"['toWireType']("+dtorStack+", arg"+i+");\n";
       args1.push("argType"+i);
     }
 
@@ -240,7 +265,7 @@ var LibraryEmbindShared = {
       for (var i = isClassMethodFunc?1:2; i < argTypes.length; ++i) { // Skip return value at index 0 - it's not deleted here. Also skip class type if not a method.
         var paramName = (i === 1 ? "thisWired" : ("arg"+(i - 2)+"Wired"));
         if (argTypes[i].destructorFunction !== null) {
-          invokerFnBody += paramName+"_dtor("+paramName+"); // "+argTypes[i].name+"\n";
+          invokerFnBody += paramName+"_dtor("+paramName+");\n";
           args1.push(paramName+"_dtor");
         }
       }
@@ -269,7 +294,7 @@ var LibraryEmbindShared = {
     invokerFnBody += "}\n";
 
 #if ASSERTIONS
-    invokerFnBody = `if (arguments.length !== ${args1.length}){ throw new Error("${humanName} Expected ${args1.length} closure arguments " + arguments.length + " given."); }\n${invokerFnBody}`;
+    invokerFnBody = `if (arguments.length !== ${args1.length}){ throw new Error(humanName + "Expected ${args1.length} closure arguments " + arguments.length + " given."); }\n${invokerFnBody}`;
 #endif
     return [args1, invokerFnBody];
   }

diff --git a/test/other/embind_jsgen_static_init.cpp b/test/other/embind_jsgen_static_init.cpp
@@ -0,0 +1,23 @@
+#include <stdio.h>
+#include <fstream>
+#include <emscripten.h>
+#include <emscripten/bind.h>
+
+using namespace emscripten;
+
+class Foo {
+public:
+  void go(const std::string& filename) {
+    std::ifstream ifs(filename, std::ifstream::binary);
+    ifs.close();
+  }
+};
+
+int main() {
+  printf("done\n");
+}
+
+EMSCRIPTEN_BINDINGS(xxx) {
+    class_<Foo>("Foo")
+      .function("go", &Foo::go);
+}
diff --git a/test/test_other.py b/test/test_other.py
@@ -3129,6 +3129,11 @@ def test_embind_tsgen_exceptions(self):
                       '-lembind', '--embind-emit-tsd', 'embind_tsgen.d.ts', '-fwasm-exceptions', '-sASSERTIONS'])
     self.assertFileContents(test_file('other/embind_tsgen.d.ts'), read_file('embind_tsgen.d.ts'))
 
+  def test_embind_jsgen_static_init(self):
+    self.emcc_args += ['-lembind', '-sEMBIND_AOT']
+
+    self.do_runf('other/embind_jsgen_static_init.cpp', 'done')
+
   def test_emconfig(self):
     output = self.run_process([emconfig, 'LLVM_ROOT'], stdout=PIPE).stdout.strip()
     self.assertEqual(output, config.LLVM_ROOT)