embind: Fix method pointers for AOT JS generation. (#21168)

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 malloc different sizes 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 #20994
emscripten-core · Jan 26, 2024 · 6ae900e · 6ae900e
1 parent 8635504
commit 6ae900e
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 13 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
@@ -7,6 +7,8 @@
 var LibraryEmbind = {
 
   $moduleDefinitions: [],
+  // Function signatures that have already been generated for JS generation.
+  $emittedFunctions: 'new Set()',
 
   $PrimitiveType: class {
     constructor(typeId, name, destructorType) {
@@ -40,7 +42,7 @@ var LibraryEmbind = {
       this.destructorType = 'none'; // Same as emval.
     }
   },
-  $FunctionDefinition__deps: ['$createJsInvoker'],
+  $FunctionDefinition__deps: ['$createJsInvoker', '$createJsInvokerSignature', '$emittedFunctions'],
   $FunctionDefinition: class {
     constructor(name, returnType, argumentTypes, functionIndex, thisType = null, isAsync = false) {
       this.name = name;
@@ -105,10 +107,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 (emittedFunctions.has(signature)) {
+        return;
+      }
+      emittedFunctions.add(signature);
+      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}},`
       );
     }
   },

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 (let 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_method_pointer_stability.cpp b/test/other/embind_jsgen_method_pointer_stability.cpp
@@ -0,0 +1,32 @@
+#include <stdio.h>
+#include <emscripten.h>
+#include <emscripten/bind.h>
+
+using namespace emscripten;
+
+__attribute__((constructor))
+void malloc_in_static_constructor(void) {
+  // Malloc different sizes in AOT generation mode and normal mode to test
+  // that method pointers still work correctly.
+  EM_ASM(
+    if (typeof InvokerFunctions == 'undefined') {
+      _malloc(10);
+    } else {
+      _malloc(100);
+    }
+  );
+}
+
+class Foo {
+public:
+  void go() {}
+};
+
+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
@@ -3130,6 +3130,12 @@ 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_method_pointer_stability(self):
+    self.emcc_args += ['-lembind', '-sEMBIND_AOT']
+    # Test that when method pointers are allocated at different addresses that
+    # AOT JS generation still works correctly.
+    self.do_runf('other/embind_jsgen_method_pointer_stability.cpp', 'done')
+
   def test_emconfig(self):
     output = self.run_process([emconfig, 'LLVM_ROOT'], stdout=PIPE).stdout.strip()
     self.assertEqual(output, config.LLVM_ROOT)