-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Stack-allocate trivial value_object/value_array argument temporaries #27610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
874a365
8c8a0cf
a1dd222
3a1a33b
86e6424
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -662,6 +662,7 @@ var LibraryEmbind = { | |
| // craftInvokerFunction generates the JS invoker function for each function exposed to JS through embind. | ||
| $craftInvokerFunction__deps: [ | ||
| '$createNamedFunction', '$runDestructors', '$throwBindingError', '$usesDestructorStack', | ||
| '$argsUseStackAlloc', '$stackSave', '$stackRestore', | ||
| #if DYNAMIC_EXECUTION && !EMBIND_AOT | ||
| '$createJsInvoker', | ||
| #endif | ||
|
|
@@ -709,6 +710,18 @@ var LibraryEmbind = { | |
| // TODO: Remove this completely once all function invokers are being dynamically generated. | ||
| var needsDestructorStack = usesDestructorStack(argTypes); | ||
|
|
||
| // Stack-allocating trivial value types get a stackSave/stackRestore | ||
| // bracket around the call; see createJsInvoker for the async carve-outs. | ||
| var argsNeedStack = argsUseStackAlloc(argTypes); | ||
| #if ASYNCIFY == 1 | ||
| var useStackFrame = false; | ||
| #else | ||
| var useStackFrame = argsNeedStack && !isAsync && !needsDestructorStack; | ||
| #endif | ||
| if (argsNeedStack && !useStackFrame) { | ||
| needsDestructorStack = true; | ||
| } | ||
|
|
||
| var returns = !argTypes[0].isVoid; | ||
|
|
||
| var expectedArgCount = argCount - 2; | ||
|
|
@@ -727,19 +740,36 @@ var LibraryEmbind = { | |
| Module.emscripten_trace_enter_context(`embind::${humanName}`); | ||
| #endif | ||
| destructors.length = 0; | ||
| var thisWired; | ||
| invokerFuncArgs.length = isClassMethodFunc ? 2 : 1; | ||
| invokerFuncArgs[0] = cppTargetFunc; | ||
| if (isClassMethodFunc) { | ||
| thisWired = argTypes[1].toWireType(destructors, this); | ||
| invokerFuncArgs[1] = thisWired; | ||
| } | ||
| for (var i = 0; i < expectedArgCount; ++i) { | ||
| argsWired[i] = argTypes[i + 2].toWireType(destructors, args[i]); | ||
| invokerFuncArgs.push(argsWired[i]); | ||
| var sp; | ||
| if (useStackFrame) { | ||
| sp = stackSave(); | ||
| } | ||
| var thisWired; | ||
| var rv; | ||
| // The frame must be released on every completion, including a throwing | ||
| // argument conversion or callee: a skipped stackRestore permanently | ||
| // leaks wasm stack. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this true? In other places in JS we don't restore the stack on the exception path (see So maybe this try/catch can be removed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's worth keeping. The case it covers is a JS getter throwing during argument conversion, after stackAlloc already ran for that argument and with every wasm call so far returned cleanly. I tried it with the restore on the normal path only and the 1000-iteration getter loop in the new test leaks 16000 bytes. It's the same story for a C++ exception escaping to JS under wasm EH (a small probe leaks 32 bytes per throw) and the exceptions docs recommend exactly this stackSave/stackRestore around the catch for that (https://emscripten.org/docs/porting/exceptions.html#handling-c-exceptions-from-javascript) withStackSave is probably fine promising less since its callers are runtime internals, but here the throws are things callers catch and carry on from, so the leak adds up.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But wouldn't anyone actually catching a C exception from JS need to restore the SP themselves? This restore might work for the leaf function but what if there is other LLVM stuff on the stack when the exception is thrown. IIUC it should be up to the catcher to restore the stack since LLVM stack frames don't restore as they unwind.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, for a C++ exception the catcher has to restore anyway since the frames between don't. The case I care about is a plain JS throw during argument conversion (a getter throwing, or a later argument failing after an earlier one already took its frame) where no wasm frame is unwinding at all. Before this change an embind call never moved the stack pointer, so a JS-side throw couldn't move it either, and a JS caller has no reason to wrap an embind call in stackSave/stackRestore. The finally keeps that property, without it the getter loop in the test drifts by one frame per throw (16 bytes for the test's type). Happy to narrow the comment to say that's what it's for.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But your Imagine for example that The corollary of that is that code within the module should not need to worry itself about restoring the stack pointer when exceptions are thrown. Even trying to make a best effort to do this in some cases I think just muddies the water.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on the rule. Once wasm has run, whoever catches owns the stack pointer, and embind shouldn't half-promise cleanup for exceptions coming out of the module. I'll drop the try/finally around the call. The one case I'd still like to cover is different from that. A conversion error (missing field, or a getter throwing) happens in embind's own JS before the C++ function is entered, so from the caller's side no module code ran and there's nothing they'd know to restore. Before this change an embind call never moved the stack pointer, so that throw couldn't leak, and it's the case the getter loop in the test measures (16 bytes per throw without a restore). I could restore in a catch around the argument conversion only and leave the call itself as you describe, which keeps the rule intact, exceptions out of the module are the caller's, exceptions before it never touch the stack. If you'd rather keep embind out of it entirely, I'll drop that too and take the throwing section out of the test.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we can/should consider conversion errors that happen at the boundary any differencly to wasm calls themselves. i.e. anyone wanting to recover from such error should really be doing stackRestore, just in case. This is how I see it: As a general rule, if you call into emscripten generated code (either native wasm code, or JS library code, or embind wrapper code, it doesn't matter) and you want to recover from a thrown exception you should always handle the stack restoration before calling back in to the module code. Emscripten does not, as a rule, take care of the shadow stack restoration during exception unwinding. Its up to each try/catch to handle that at the catch site.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the documenation was not completely clear on this I created #27748. I think this accurately reflects the reality.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, and the docs change settles it. I took the try/finally out and restore on the normal path only, and dropped the throwing section of the test: #27750 |
||
| try { | ||
| invokerFuncArgs.length = isClassMethodFunc ? 2 : 1; | ||
| invokerFuncArgs[0] = cppTargetFunc; | ||
| if (isClassMethodFunc) { | ||
| thisWired = argTypes[1].toWireType(destructors, this); | ||
| invokerFuncArgs[1] = thisWired; | ||
| } | ||
| for (var i = 0; i < expectedArgCount; ++i) { | ||
| var argType = argTypes[i + 2]; | ||
| // Stack-allocating types take the stack path only under a frame; a | ||
| // null destructors argument is that contract. | ||
| argsWired[i] = argType.toWireType(useStackFrame && argType.argStackAlloc ? null : destructors, args[i]); | ||
| invokerFuncArgs.push(argsWired[i]); | ||
| } | ||
|
|
||
| var rv = cppInvokerFunc(...invokerFuncArgs); | ||
| rv = cppInvokerFunc(...invokerFuncArgs); | ||
| } finally { | ||
| if (useStackFrame) { | ||
| stackRestore(sp); | ||
| } | ||
| } | ||
|
|
||
| function onDone(rv) { | ||
| if (needsDestructorStack) { | ||
|
|
@@ -780,6 +810,10 @@ var LibraryEmbind = { | |
| var retType = argTypes[0]; | ||
| var instType = argTypes[1]; | ||
| var closureArgs = [humanName, throwBindingError, cppInvokerFunc, cppTargetFunc, runDestructors, retType.fromWireType.bind(retType), instType?.toWireType.bind(instType)]; | ||
| if (useStackFrame) { | ||
| // Must mirror the `args1.push('stackSave', 'stackRestore')` in createJsInvoker. | ||
| closureArgs.push(stackSave, stackRestore); | ||
| } | ||
| #if EMSCRIPTEN_TRACING | ||
| closureArgs.push(Module); | ||
| #endif | ||
|
|
@@ -887,12 +921,16 @@ var LibraryEmbind = { | |
| constructorSignature, | ||
| rawConstructor, | ||
| destructorSignature, | ||
| rawDestructor | ||
| rawDestructor, | ||
| valueSize, | ||
| isTrivial | ||
| ) => { | ||
| tupleRegistrations[rawType] = { | ||
| name: AsciiToString(name), | ||
| rawConstructor: embind__requireFunction(constructorSignature, rawConstructor), | ||
| rawDestructor: embind__requireFunction(destructorSignature, rawDestructor), | ||
| valueSize, | ||
| isTrivial: !!isTrivial, | ||
| elements: [], | ||
| }; | ||
| }, | ||
|
|
@@ -922,7 +960,7 @@ var LibraryEmbind = { | |
|
|
||
| _embind_finalize_value_array__deps: [ | ||
| '$tupleRegistrations', '$runDestructors', | ||
| '$readPointer', '$whenDependentTypesAreResolved'], | ||
| '$readPointer', '$whenDependentTypesAreResolved', '$stackAlloc'], | ||
| _embind_finalize_value_array: (rawTupleType) => { | ||
| var reg = tupleRegistrations[rawTupleType]; | ||
| delete tupleRegistrations[rawTupleType]; | ||
|
|
@@ -933,6 +971,8 @@ var LibraryEmbind = { | |
|
|
||
| var rawConstructor = reg.rawConstructor; | ||
| var rawDestructor = reg.rawDestructor; | ||
| var valueSize = reg.valueSize; | ||
| var isTrivial = reg.isTrivial; | ||
|
|
||
| whenDependentTypesAreResolved([rawTupleType], elementTypes, (elementTypes) => { | ||
| for (const [i, elt] of elements.entries()) { | ||
|
|
@@ -943,11 +983,19 @@ var LibraryEmbind = { | |
| const setter = elt.setter; | ||
| const setterContext = elt.setterContext; | ||
| elt.read = (ptr) => getterReturnType.fromWireType(getter(getterContext, ptr)); | ||
| elt.write = (ptr, o) => { | ||
| var destructors = []; | ||
| setter(setterContext, ptr, setterArgumentType.toWireType(destructors, o)); | ||
| runDestructors(destructors); | ||
| }; | ||
| if (setterArgumentType.destructorFunction === null && !setterArgumentType.argStackAlloc) { | ||
| // The element type never registers a destructor, so skip the | ||
| // per-write destructors array. (Stack-allocating types still need | ||
| // the array here: a null destructors argument means an | ||
| // invoker-managed stack frame, which a nested write cannot assume.) | ||
| elt.write = (ptr, o) => setter(setterContext, ptr, setterArgumentType.toWireType(null, o)); | ||
| } else { | ||
| elt.write = (ptr, o) => { | ||
| var destructors = []; | ||
| setter(setterContext, ptr, setterArgumentType.toWireType(destructors, o)); | ||
| runDestructors(destructors); | ||
| }; | ||
| } | ||
|
dimokol marked this conversation as resolved.
|
||
| } | ||
|
|
||
| return [{ | ||
|
|
@@ -964,17 +1012,33 @@ var LibraryEmbind = { | |
| if (elementsLength !== o.length) { | ||
| throw new TypeError(`Incorrect number of tuple elements for ${reg.name}: expected=${elementsLength}, actual=${o.length}`); | ||
| } | ||
| var ptr = rawConstructor(); | ||
| var ptr; | ||
| if (isTrivial && destructors === null) { | ||
| // Trivially constructible and destructible, and the invoker | ||
| // manages a stack frame around this call: the temporary lives on | ||
| // the wasm stack. No allocation, nothing to destruct. Callers | ||
| // that defer destruction (emval returns, property setters) pass | ||
| // a destructors array instead and take the heap path below. | ||
| // Zero-fill so unregistered fields and padding match the | ||
| // value-initialization the heap path's `new T()` performs. | ||
| ptr = stackAlloc(valueSize); | ||
| HEAPU8.fill(0, ptr, ptr + valueSize); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missed this. Switching both spots over in a followup |
||
| } else { | ||
| ptr = rawConstructor(); | ||
| if (destructors !== null) { | ||
| destructors.push(rawDestructor, ptr); | ||
| } | ||
| } | ||
| for (var i = 0; i < elementsLength; ++i) { | ||
| elements[i].write(ptr, o[i]); | ||
| } | ||
| if (destructors !== null) { | ||
| destructors.push(rawDestructor, ptr); | ||
| } | ||
| return ptr; | ||
| }, | ||
| readValueFromPointer: readPointer, | ||
| destructorFunction: rawDestructor, | ||
| // Trivial types have nothing to run after the call: the stack frame | ||
| // (or the destructors array, on the deferred path) covers cleanup. | ||
| destructorFunction: isTrivial ? null : rawDestructor, | ||
| argStackAlloc: isTrivial, | ||
| }]; | ||
| }); | ||
| }, | ||
|
|
@@ -987,12 +1051,16 @@ var LibraryEmbind = { | |
| constructorSignature, | ||
| rawConstructor, | ||
| destructorSignature, | ||
| rawDestructor | ||
| rawDestructor, | ||
| valueSize, | ||
| isTrivial | ||
| ) => { | ||
| structRegistrations[rawType] = { | ||
| name: AsciiToString(name), | ||
| rawConstructor: embind__requireFunction(constructorSignature, rawConstructor), | ||
| rawDestructor: embind__requireFunction(destructorSignature, rawDestructor), | ||
| valueSize, | ||
| isTrivial: !!isTrivial, | ||
| fields: [], | ||
| }; | ||
| }, | ||
|
|
@@ -1024,13 +1092,15 @@ var LibraryEmbind = { | |
|
|
||
| _embind_finalize_value_object__deps: [ | ||
| '$structRegistrations', '$runDestructors', | ||
| '$readPointer', '$whenDependentTypesAreResolved'], | ||
| '$readPointer', '$whenDependentTypesAreResolved', '$stackAlloc'], | ||
| _embind_finalize_value_object: (structType) => { | ||
| var reg = structRegistrations[structType]; | ||
| delete structRegistrations[structType]; | ||
|
|
||
| var rawConstructor = reg.rawConstructor; | ||
| var rawDestructor = reg.rawDestructor; | ||
| var valueSize = reg.valueSize; | ||
| var isTrivial = reg.isTrivial; | ||
| var fieldRecords = reg.fields; | ||
| var fieldTypes = fieldRecords.map((field) => field.getterReturnType). | ||
| concat(fieldRecords.map((field) => field.setterArgumentType)); | ||
|
|
@@ -1043,13 +1113,20 @@ var LibraryEmbind = { | |
| const setterArgumentType = fieldTypes[i + fieldRecords.length]; | ||
| const setter = field.setter; | ||
| const setterContext = field.setterContext; | ||
| fields[field.fieldName] = { | ||
| read: (ptr) => getterReturnType.fromWireType(getter(getterContext, ptr)), | ||
| write: (ptr, o) => { | ||
| var write; | ||
| if (setterArgumentType.destructorFunction === null && !setterArgumentType.argStackAlloc) { | ||
| // See the matching element-write logic in _embind_finalize_value_array. | ||
| write = (ptr, o) => setter(setterContext, ptr, setterArgumentType.toWireType(null, o)); | ||
| } else { | ||
| write = (ptr, o) => { | ||
| var destructors = []; | ||
| setter(setterContext, ptr, setterArgumentType.toWireType(destructors, o)); | ||
| runDestructors(destructors); | ||
| }, | ||
| }; | ||
| } | ||
| fields[field.fieldName] = { | ||
| read: (ptr) => getterReturnType.fromWireType(getter(getterContext, ptr)), | ||
| write, | ||
| optional: getterReturnType.optional, | ||
| }; | ||
| } | ||
|
|
@@ -1072,17 +1149,28 @@ var LibraryEmbind = { | |
| throw new TypeError(`Missing field: "${fieldName}"`); | ||
| } | ||
| } | ||
| var ptr = rawConstructor(); | ||
| var ptr; | ||
| if (isTrivial && destructors === null) { | ||
| // See the matching branch in _embind_finalize_value_array: the | ||
| // invoker manages a stack frame, so the temporary lives on the | ||
| // wasm stack with no allocation and no destructor bookkeeping; | ||
| // zero-filled to match the heap path's value-initialization. | ||
| ptr = stackAlloc(valueSize); | ||
| HEAPU8.fill(0, ptr, ptr + valueSize); | ||
| } else { | ||
| ptr = rawConstructor(); | ||
| if (destructors !== null) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just use the truthiness of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, it's only ever an array or null. Same followup. |
||
| destructors.push(rawDestructor, ptr); | ||
| } | ||
| } | ||
| for (fieldName in fields) { | ||
| fields[fieldName].write(ptr, o[fieldName]); | ||
| } | ||
| if (destructors !== null) { | ||
| destructors.push(rawDestructor, ptr); | ||
| } | ||
| return ptr; | ||
| }, | ||
| readValueFromPointer: readPointer, | ||
| destructorFunction: rawDestructor, | ||
| destructorFunction: isTrivial ? null : rawDestructor, | ||
| argStackAlloc: isTrivial, | ||
| }]; | ||
| }); | ||
| }, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.