Skip to content

Commit

Permalink
[embind] Change how the number of arguments are verified. (#22591)
Browse files Browse the repository at this point in the history
This is really two changes, but they both will affect the same code so I
did them at once:
- The number of arguments is now only verified with ASSERTIONS enabled.
This will help code size and the speed of embind invoker functions.
 - Allow optional arguments to be omitted.

Fixes #22389
  • Loading branch information
brendandahl authored Sep 20, 2024
1 parent 57aeff1 commit 203bc12
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 16 deletions.
3 changes: 3 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ See docs/process.md for more on how version tagging works.
3.1.68 (in development)
-----------------------
- The freetype port was updated from v2.6 to v2.13.3. (#22585)
- The number of arguments passed to Embind function calls is now only verified
with ASSERTIONS enabled. (#22591)
- Optional arguments can now be omitted from Embind function calls. (#22591)

3.1.67 - 09/17/24
-----------------
Expand Down
24 changes: 18 additions & 6 deletions src/embind/embind.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ var LibraryEmbind = {
// TODO: do we need a deleteObject here? write a test where
// emval is passed into JS via an interface
}`,
$EmValOptionalType__deps: ['$EmValType'],
$EmValOptionalType: '=Object.assign({optional: true}, EmValType);',
$init_embind__deps: [
'$getInheritedInstanceCount', '$getLiveInheritedInstances',
'$flushPendingDeletes', '$setDelayFunction'],
Expand Down Expand Up @@ -687,9 +689,9 @@ var LibraryEmbind = {
__embind_register_emval(rawType);
},

_embind_register_optional__deps: ['_embind_register_emval'],
_embind_register_optional__deps: ['$registerType', '$EmValOptionalType'],
_embind_register_optional: (rawOptionalType, rawType) => {
__embind_register_emval(rawOptionalType);
registerType(rawOptionalType, EmValOptionalType);
},

_embind_register_memory_view__deps: ['$readLatin1String', '$registerType'],
Expand Down Expand Up @@ -778,6 +780,10 @@ var LibraryEmbind = {
#endif
#if ASYNCIFY
'$Asyncify',
#endif
#if ASSERTIONS
'$getRequiredArgCount',
'$checkArgCount',
#endif
],
$craftInvokerFunction: function(humanName, argTypes, classType, cppInvokerFunc, cppTargetFunc, /** boolean= */ isAsync) {
Expand Down Expand Up @@ -821,15 +827,18 @@ var LibraryEmbind = {

var returns = (argTypes[0].name !== "void");

#if DYNAMIC_EXECUTION == 0 && !EMBIND_AOT
var expectedArgCount = argCount - 2;
#if ASSERTIONS
var minArgs = getRequiredArgCount(argTypes);
#endif
#if DYNAMIC_EXECUTION == 0 && !EMBIND_AOT
var argsWired = new Array(expectedArgCount);
var invokerFuncArgs = [];
var destructors = [];
var invokerFn = function(...args) {
if (args.length !== expectedArgCount) {
throwBindingError(`function ${humanName} called with ${args.length} arguments, expected ${expectedArgCount}`);
}
#if ASSERTIONS
checkArgCount(args.length, minArgs, expectedArgCount, humanName, throwBindingError);
#endif
#if EMSCRIPTEN_TRACING
Module.emscripten_trace_enter_context(`embind::${humanName}`);
#endif
Expand Down Expand Up @@ -901,6 +910,9 @@ var LibraryEmbind = {
}
}
}
#if ASSERTIONS
closureArgs.push(checkArgCount, minArgs, expectedArgCount);
#endif

#if EMBIND_AOT
var signature = createJsInvokerSignature(argTypes, isClassMethodFunc, returns, isAsync);
Expand Down
35 changes: 29 additions & 6 deletions src/embind/embind_shared.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,29 @@ var LibraryEmbindShared = {
return signature.join('');
},

$createJsInvoker__deps: ['$usesDestructorStack'],
$checkArgCount(numArgs, minArgs, maxArgs, humanName, throwBindingError) {
if (numArgs < minArgs || numArgs > maxArgs) {
var argCountMessage = minArgs == maxArgs ? minArgs : `${minArgs} to ${maxArgs}`;
throwBindingError(`function ${humanName} called with ${numArgs} arguments, expected ${argCountMessage}`);
}
},

$getRequiredArgCount(argTypes) {
var requiredArgCount = argTypes.length - 2;
for (var i = argTypes.length - 1; i >= 2; --i) {
if (!argTypes[i].optional) {
break;
}
requiredArgCount--;
}
return requiredArgCount;
},

$createJsInvoker__deps: ['$usesDestructorStack',
#if ASSERTIONS
'$checkArgCount',
#endif
],
$createJsInvoker(argTypes, isClassMethodFunc, returns, isAsync) {
var needsDestructorStack = usesDestructorStack(argTypes);
var argCount = argTypes.length - 2;
Expand All @@ -220,11 +242,11 @@ var LibraryEmbindShared = {
argsList = argsList.join(',')
argsListWired = argsListWired.join(',')

var invokerFnBody = `
return function (${argsList}) {
if (arguments.length !== ${argCount}) {
throwBindingError('function ' + humanName + ' called with ' + arguments.length + ' arguments, expected ${argCount}');
}`;
var invokerFnBody = `return function (${argsList}) {\n`;

#if ASSERTIONS
invokerFnBody += "checkArgCount(arguments.length, minArgs, maxArgs, humanName, throwBindingError);\n";
#endif

#if EMSCRIPTEN_TRACING
invokerFnBody += `Module.emscripten_trace_enter_context('embind::' + humanName );\n`;
Expand Down Expand Up @@ -295,6 +317,7 @@ var LibraryEmbindShared = {
invokerFnBody += "}\n";

#if ASSERTIONS
args1.push('checkArgCount', 'minArgs', 'maxArgs');
invokerFnBody = `if (arguments.length !== ${args1.length}){ throw new Error(humanName + "Expected ${args1.length} closure arguments " + arguments.length + " given."); }\n${invokerFnBody}`;
#endif
return [args1, invokerFnBody];
Expand Down
8 changes: 4 additions & 4 deletions test/code_size/embind_hello_wasm.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 552,
"a.html.gz": 380,
"a.js": 9910,
"a.js.gz": 4352,
"a.js": 9727,
"a.js.gz": 4295,
"a.wasm": 7728,
"a.wasm.gz": 3519,
"total": 18190,
"total_gz": 8251
"total": 18007,
"total_gz": 8194
}
14 changes: 14 additions & 0 deletions test/embind/embind.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1294,6 +1294,20 @@ module({
value = cm.embind_test_optional_small_class_arg(undefined);
assert.equal(-1, value);
});

test("std::optional args can be omitted", function() {
if (cm.getCompilerSetting('ASSERTIONS')) {
// Argument length is only validated with assertions enabled.
assert.throws(cm.BindingError, function() {
cm.embind_test_optional_multiple_arg();
});
assert.throws(cm.BindingError, function() {
cm.embind_test_optional_multiple_arg(1, 2, 3, 4);
});
}
cm.embind_test_optional_multiple_arg(1);
cm.embind_test_optional_multiple_arg(1, 2);
});
});

BaseFixture.extend("functors", function() {
Expand Down
2 changes: 2 additions & 0 deletions test/embind/embind_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1377,6 +1377,7 @@ int embind_test_optional_small_class_arg(std::optional<SmallClass> arg) {
}
return -1;
}
void embind_test_optional_multiple_arg(int arg1, std::optional<int> arg2, std::optional<int> arg3) {}
#endif

val embind_test_getglobal() {
Expand Down Expand Up @@ -2412,6 +2413,7 @@ EMSCRIPTEN_BINDINGS(tests) {
function("embind_test_optional_float_arg", &embind_test_optional_float_arg);
function("embind_test_optional_string_arg", &embind_test_optional_string_arg);
function("embind_test_optional_small_class_arg", &embind_test_optional_small_class_arg);
function("embind_test_optional_multiple_arg", &embind_test_optional_multiple_arg);
#endif

register_map<std::string, int>("StringIntMap");
Expand Down

0 comments on commit 203bc12

Please sign in to comment.