From 1daa248174524afa4b6b53de2b7a0350d9dc59b7 Mon Sep 17 00:00:00 2001 From: Brendan Dahl Date: Thu, 25 Jan 2024 01:00:23 +0000 Subject: [PATCH] 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 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 --- src/embind/embind.js | 8 ++-- src/embind/embind_gen.js | 13 +++++-- src/embind/embind_shared.js | 39 +++++++++++++++---- .../embind_jsgen_method_pointer_stability.cpp | 32 +++++++++++++++ test/test_other.py | 6 +++ 5 files changed, 85 insertions(+), 13 deletions(-) create mode 100644 test/other/embind_jsgen_method_pointer_stability.cpp diff --git a/src/embind/embind.js b/src/embind/embind.js index 44b1131ac179d..a1c01cbcd2b5f 100644 --- 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 index d5446e6f18e4d..98735cec8fa40 100644 --- 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 index e45fb2add9acb..3c61ce07804e3 100644 --- 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,11 +218,11 @@ 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) { @@ -205,7 +230,7 @@ var LibraryEmbindShared = { } 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 new file mode 100644 index 0000000000000..8031b86fd6483 --- /dev/null +++ b/test/other/embind_jsgen_method_pointer_stability.cpp @@ -0,0 +1,32 @@ +#include +#include +#include + +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") + .function("go", &Foo::go); +} diff --git a/test/test_other.py b/test/test_other.py index 4dabc8ecb3524..b306bb1a2ddcc 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -3129,6 +3129,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)