From 3afad4d432c65aeacd038758d21f523c453c4a67 Mon Sep 17 00:00:00 2001 From: stephan Date: Sat, 1 Oct 2022 18:47:42 +0000 Subject: [PATCH] wasm: correct a memleak caused by a shadowed var in the previous checkin. Add a stack-like allocator, sqlite3.capi.wasm.pstack, as a faster way of managing short-lived pointers (like the one which got shadowed). FossilOrigin-Name: 1fa019c88deac6b6e5155b620bdab1d7145846290daafb9adbefcf4f0fe542cf --- ext/wasm/api/sqlite3-api-prologue.js | 106 +++++++++++++++++++++------ ext/wasm/api/sqlite3-wasm.c | 105 ++++++++++++++++++++++++++ ext/wasm/testing1.js | 38 +++++++++- manifest | 16 ++-- manifest.uuid | 2 +- 5 files changed, 232 insertions(+), 35 deletions(-) diff --git a/ext/wasm/api/sqlite3-api-prologue.js b/ext/wasm/api/sqlite3-api-prologue.js index 121b3d4ffa..0e4a72ab65 100644 --- a/ext/wasm/api/sqlite3-api-prologue.js +++ b/ext/wasm/api/sqlite3-api-prologue.js @@ -738,30 +738,92 @@ self.sqlite3ApiBootstrap = function sqlite3ApiBootstrap( ["sqlite3_wasm_vfs_unlink", "int", "string"] ]; + + /** + sqlite3.capi.wasm.pstack (pseudo-stack) holds a special-case + stack-style allocator intended only for use with _small_ data of + not more than (in total) a few kb in size, managed as if it were + stack-based. + + It has only a single intended usage: + + ``` + const stackPos = pstack.pointer; + try{ + const ptr = pstack.alloc(8); + // ==> pstack.pointer === ptr + const otherPtr = pstack.alloc(8); + // ==> pstack.pointer === otherPtr + ... + }finally{ + pstack.restore(stackPos); + // ==> pstack.pointer === stackPos + } + ``` + + This allocator is much faster than a general-purpose one but is + limited to usage patterns like the one shown above. + + It operates from a static range of memory which lives outside of + space managed by Emscripten's stack-management, so does not + collide with Emscripten-provided stack allocation APIs. The + memory lives in the WASM heap and can be used with routines such + as wasm.setMemValue() and any wasm.heap8u().slice(). + */ + capi.wasm.pstack = Object.assign(Object.create(null),{ + /** + Sets the current ppstack position to the given pointer. + Results are undefined if the passed-in value did not come from + this.pointer. + */ + restore: capi.wasm.exports.sqlite3_wasm_pstack_restore, + /** + Attempts to allocate the given number of bytes from the + pstack. On success, it zeroes out a block of memory of the + given size, adjusts the pstack pointer, and returns a pointer + to the memory. On error, returns 0. The memory must eventually + be released using restore(). + + This method always adjusts the given value to be a multiple + of 8 in order to keep alignment guarantees. + */ + alloc: capi.wasm.exports.sqlite3_wasm_pstack_alloc + }); + /** + sqlite3.capi.wasm.pstack.pointer resolves to the current pstack + position pointer. This value is intended _only_ to be passed to restore(). + */ + Object.defineProperty(capi.wasm.pstack, 'pointer', { + configurable: false, iterable: true, writeable: false, + get: capi.wasm.exports.sqlite3_wasm_pstack_ptr + //Whether or not a setter as an alternative to restore() is + //clearer or would just lead to confusion is unclear. + //set: capi.wasm.exports.sqlite3_wasm_pstack_restore + }); + /** + sqlite3.capi.wasm.pstack.remaining resolves to the amount of + space remaining in the pstack. + */ + Object.defineProperty(capi.wasm.pstack, 'remaining', { + configurable: false, iterable: true, writeable: false, + get: capi.wasm.exports.sqlite3_wasm_pstack_remaining + }); + + /** State for sqlite3_wasmfs_opfs_dir(). */ let __persistentDir = undefined; /** - An experiment. Do not use in client code. - - If the wasm environment has a persistent storage directory, - its path is returned by this function. If it does not then - it returns "" (noting that "" is a falsy value). + If the wasm environment has a WASMFS/OPFS-backed persistent + storage directory, its path is returned by this function. If it + does not then it returns "" (noting that "" is a falsy value). The first time this is called, this function inspects the current - environment to determine whether persistence filesystem support - is available and, if it is, enables it (if needed). + environment to determine whether persistence support is available + and, if it is, enables it (if needed). This function currently only recognizes the WASMFS/OPFS storage - combination. "Plain" OPFS is provided via a separate VFS which - is optionally be installed via sqlite3.asyncPostInit(). - - TODOs and caveats: - - - If persistent storage is available at the root of the virtual - filesystem, this interface cannot currently distinguish that - from the lack of persistence. That can (in the mean time) - happen when using the JS-native "opfs" VFS, as opposed to the - WASMFS/OPFS combination. + combination and its path refers to storage rooted in the + Emscripten-managed virtual filesystem. */ capi.sqlite3_wasmfs_opfs_dir = function(){ if(undefined !== __persistentDir) return __persistentDir; @@ -879,10 +941,10 @@ self.sqlite3ApiBootstrap = function sqlite3ApiBootstrap( if(!pDb) toss('Invalid sqlite3* argument.'); const wasm = capi.wasm; if(!wasm.bigIntEnabled) toss('BigInt64 support is not enabled.'); - const scope = wasm.scopedAllocPush(); + const stack = wasm.pstack.pointer(); let pOut; try{ - const pSize = wasm.scopedAlloc(8/*i64*/ + wasm.ptrSizeof); + const pSize = wasm.pstack.alloc(8/*i64*/ + wasm.ptrSizeof); const ppOut = pSize + 8; /** Maintenance reminder, since this cost a full hour of grief @@ -891,8 +953,6 @@ self.sqlite3ApiBootstrap = function sqlite3ApiBootstrap( export reads a garbage size because it's not on an 8-byte memory boundary! */ - wasm.setPtrValue(ppOut, 0); - wasm.setMemValue(pSize, 0, 'i64'); let rc = wasm.exports.sqlite3_wasm_db_serialize( pDb, ppOut, pSize, 0 ); @@ -900,7 +960,7 @@ self.sqlite3ApiBootstrap = function sqlite3ApiBootstrap( toss("Database serialization failed with code", sqlite3.capi.sqlite3_web_rc_str(rc)); } - const pOut = wasm.getPtrValue(ppOut); + pOut = wasm.getPtrValue(ppOut); const nOut = wasm.getMemValue(pSize, 'i64'); rc = nOut ? wasm.heap8u().slice(pOut, pOut + Number(nOut)) @@ -911,7 +971,7 @@ self.sqlite3ApiBootstrap = function sqlite3ApiBootstrap( throw w; }finally{ if(pOut) wasm.exports.sqlite3_free(pOut); - wasm.scopedAllocPop(scope); + wasm.pstack.restore(stack); } }; diff --git a/ext/wasm/api/sqlite3-wasm.c b/ext/wasm/api/sqlite3-wasm.c index a66b51a8fc..4cb8cc2643 100644 --- a/ext/wasm/api/sqlite3-wasm.c +++ b/ext/wasm/api/sqlite3-wasm.c @@ -56,6 +56,7 @@ # define SQLITE_THREADSAFE 0 #endif +#include #include "sqlite3.c" /* yes, .c instead of .h. */ /* @@ -85,6 +86,110 @@ // See also: //__attribute__((export_name("theExportedName"), used, visibility("default"))) + +#if 0 +/* +** An EXPERIMENT in implementing a stack-based allocator analog to +** Emscripten's stackSave(), stackAlloc(), stackRestore(). +** Unfortunately, this cannot work together with Emscripten because +** Emscripten defines its own native one and we'd stomp on each +** other's memory. Other than that complication, basic tests show it +** to work just fine. +** +** Another option is to malloc() a chunk of our own and call that our +** "stack". +*/ +WASM_KEEP void * sqlite3_wasm_stack_end(void){ + extern void __heap_base + /* see https://stackoverflow.com/questions/10038964 */; + return &__heap_base; +} +WASM_KEEP void * sqlite3_wasm_stack_begin(void){ + extern void __data_end; + return &__data_end; +} +static void * sq3StackPtr = 0; +WASM_KEEP void * sqlite3_wasm_stack_ptr(void){ + if(!sq3StackPtr) sq3StackPtr = sqlite3_wasm_stack_end(); + return sq3StackPtr; +} +WASM_KEEP void sqlite3_wasm_stack_restore(void * p){ + sq3StackPtr = p; +} +WASM_KEEP void * sqlite3_wasm_stack_alloc(int n){ + if(n<=0) return 0; + n = (n + 7) & ~7 /* align to 8-byte boundary */; + unsigned char * const p = (unsigned char *)sqlite3_wasm_stack_ptr(); + unsigned const char * const b = (unsigned const char *)sqlite3_wasm_stack_begin(); + if(b + n >= p || b + n < b/*overflow*/) return 0; + return sq3StackPtr = p - n; +} +#endif /* stack allocator experiment */ + +/* +** State for the "pseudo-stack" allocator implemented in +** sqlite3_wasm_pstack_xyz(). In order to avoid colliding with +** Emscripten-controled stack space, it carves out a bit of stack +** memory to use for that purpose. This memory ends up in the +** WASM-managed memory, such that routines which manipulate the wasm +** heap can also be used to manipulate this memory. +*/ +static unsigned char PStack_mem[512 * 8] = {0}; +static struct { + unsigned char const * pBegin; /* Start (inclusive) of memory range */ + unsigned char const * pEnd; /* One-after-the-end of memory range */ + unsigned char * pPos; /* Current "stack pointer" */ +} PStack = { + &PStack_mem[0], + &PStack_mem[0] + sizeof(PStack_mem), + &PStack_mem[0] + sizeof(PStack_mem) +}; +/* +** Returns the current pstack position. +*/ +WASM_KEEP void * sqlite3_wasm_pstack_ptr(void){ + return PStack.pPos; +} +/* +** Sets the pstack position poitner to p. Results are undefined if the +** given value did not come from sqlite3_wasm_pstack_ptr(). +*/ +WASM_KEEP void sqlite3_wasm_pstack_restore(unsigned char * p){ + assert(p>=PStack.pBegin && p<=PStack.pEnd && p>=PStack.pPos); + assert(0==(p & 0x7)); + if(p>=PStack.pBegin && p<=PStack.pEnd /*&& p>=PStack.pPos*/){ + PStack.pPos = p; + } +} +/* +** Allocate and zero out n bytes from the pstack. Returns a pointer to +** the memory on success, 0 on error (including a negative n value). n +** is always adjusted to be a multiple of 8 and returned memory is +** always zeroed out before returning (because this keeps the client +** JS code from having to do so, and most uses of the pstack will +** call for doing so). +*/ +WASM_KEEP void * sqlite3_wasm_pstack_alloc(int n){ + if( n<=0 ) return 0; + //if( n & 0x7 ) n += 8 - (n & 0x7) /* align to 8-byte boundary */; + n = (n + 7) & ~7 /* align to 8-byte boundary */; + unsigned char * const p = PStack.pPos; + unsigned const char * const b = PStack.pBegin; + if( b + n > p || b + n <= b/*overflow*/ ) return 0; + memset((PStack.pPos = p - n), 0, (unsigned int)n); + return PStack.pPos; +} +/* +** Return the number of bytes left which can be +** sqlite3_wasm_pstack_alloc()'d. +*/ +WASM_KEEP int sqlite3_wasm_pstack_remaining(void){ + assert(PStack.pPos >= PStack.pBegin); + assert(PStack.pPos <= PStack.pEnd); + return (int)(PStack.pPos - PStack.pBegin); +} + + /* ** This function is NOT part of the sqlite3 public API. It is strictly ** for use by the sqlite project's own JS/WASM bindings. diff --git a/ext/wasm/testing1.js b/ext/wasm/testing1.js index 916f81babc..ffe63c537c 100644 --- a/ext/wasm/testing1.js +++ b/ext/wasm/testing1.js @@ -875,7 +875,7 @@ log("cstrncpy()..."); { - w.scopedAllocPush(); + const scope = w.scopedAllocPush(); try { let cStr = w.scopedAllocCString("hello"); const n = w.cstrlen(cStr); @@ -892,7 +892,7 @@ assert(chr('!') === w.getMemValue(cpy+2)). assert(chr('l') === w.getMemValue(cpy+3)); }finally{ - w.scopedAllocPop(); + w.scopedAllocPop(scope); } } @@ -1024,6 +1024,38 @@ } }/*testWasmUtil()*/; + + /** + Tests for sqlite3.capi.wasm.pstack(). + */ + const testPstack = function(db,sqlite3){ + const w = sqlite3.capi.wasm, P = w.pstack; + const stack = P.pointer; + T.assert(0===stack % 8 /* must be 8-byte aligned */); + try{ + const quota = P.remaining; + log("pstack quota",quota); + T.assert(quota >= 4096) + .assert(0 === P.alloc(0)) + .assert(0 === P.alloc(-1)); + let p1 = P.alloc(12); + T.assert(p1 === stack - 16/*8-byte aligned*/) + .assert(P.pointer === p1); + let p2 = P.alloc(7); + T.assert(p2 === p1-8/*8-byte aligned, stack grows downwards*/) + .assert(0 === P.alloc(quota)) + .assert(24 === stack - p2) + .assert(P.pointer === p2); + let n = quota - (stack - p2); + let p3 = P.alloc(n); + T.assert(p3 === stack-quota) + .assert(0 === P.alloc(1)); + }finally{ + P.restore(stack); + T.assert(P.pointer === stack); + } + }/*testPstack()*/; + const clearKvvfs = function(){ const sz = sqlite3.capi.sqlite3_web_kvvfs_size(); const n = sqlite3.capi.sqlite3_web_kvvfs_clear(''); @@ -1089,7 +1121,7 @@ [ testWasmUtil, testBasicSanity, testUDF, testAttach, testIntPtr, testStructStuff, - testSqliteStructs + testSqliteStructs, testPstack ].forEach((f)=>{ const t = T.counter, n = performance.now(); logHtml(banner1,"Running",f.name+"()..."); diff --git a/manifest b/manifest index b5ccfef55a..4898d8f756 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Fiddle:\sfix\smakefile\sdependency\sissue\sand\sduplicate\sinclusion\sof\spost-js.js.\sReimplement\sdb\sexport\susing\ssqlite3_serialize(). -D 2022-10-01T16:01:41.242 +C wasm:\scorrect\sa\smemleak\scaused\sby\sa\sshadowed\svar\sin\sthe\sprevious\scheckin.\sAdd\sa\sstack-like\sallocator,\ssqlite3.capi.wasm.pstack,\sas\sa\sfaster\sway\sof\smanaging\sshort-lived\spointers\s(like\sthe\sone\swhich\sgot\sshadowed). +D 2022-10-01T18:47:42.149 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724 @@ -488,10 +488,10 @@ F ext/wasm/api/sqlite3-api-cleanup.js 5d22d1d3818ecacb23bfa223d5970cd0617d8cdbb4 F ext/wasm/api/sqlite3-api-glue.js b15a51b88aaa472d36bf82d5123dbfdafe8ddf6ca75fba934510e4a20bbe4adb F ext/wasm/api/sqlite3-api-oo1.js 9caed0757a5e039ed92467e827fd3ca347fa08f19fe086fcbdd14a4ebe9c2f01 F ext/wasm/api/sqlite3-api-opfs.js 1b097808b7b081b0f0700cf97d49ef19760e401706168edff9cd45cf9169f541 -F ext/wasm/api/sqlite3-api-prologue.js d7904da82691f68b9ffb081c072cf4725716154284a63447f2d1dce40ab4b85f +F ext/wasm/api/sqlite3-api-prologue.js 895b5f9ee425b8ba4a5fc5c85f144da9e2b785fea9226830f068d5c3b71e0fba F ext/wasm/api/sqlite3-api-worker1.js 7f4f46cb6b512a48572d7567233896e6a9c46570c44bdc3d13419730c7c221c8 F ext/wasm/api/sqlite3-wasi.h 25356084cfe0d40458a902afb465df8c21fc4152c1d0a59b563a3fba59a068f9 -F ext/wasm/api/sqlite3-wasm.c 61c6bf8404a07f3d5f2861fbc1d3596474cecfc38801fb3a38c560fe847f79a5 +F ext/wasm/api/sqlite3-wasm.c 43216a5266780f5f6ca3972684be5cd9113b624e43a5f5923c06a0c386e65857 F ext/wasm/batch-runner.html c363032aba7a525920f61f8be112a29459f73f07e46f0ba3b7730081a617826e F ext/wasm/batch-runner.js ce92650a6681586c89bef26ceae96674a55ca5a9727815202ca62e1a00ff5015 F ext/wasm/common/SqliteTestUtil.js 647bf014bd30bdd870a7e9001e251d12fc1c9ec9ce176a1004b838a4b33c5c05 @@ -530,7 +530,7 @@ F ext/wasm/test-opfs-vfs.js a59ff9210b17d46b0c6fbf6a0ba60143c033327865f2e556e14f F ext/wasm/testing-worker1-promiser.html 6eaec6e04a56cf24cf4fa8ef49d78ce8905dde1354235c9125dca6885f7ce893 F ext/wasm/testing-worker1-promiser.js bd788e33c1807e0a6dda9c9a9d784bd3350ca49c9dd8ae2cc8719b506b6e013e F ext/wasm/testing1.html 50575755e43232dbe4c2f97c9086b3118eb91ec2ee1fae931e6d7669fb17fcae -F ext/wasm/testing1.js 5584e9b68f797dbc0f6161360f2c6840169533813d92c74d355a3e78dd5bb539 +F ext/wasm/testing1.js 419e029ee4ae3cf98dd4c20aebdb111afe743b54ed8856e5f59775c3241fd21f F ext/wasm/testing2.html a66951c38137ff1d687df79466351f3c734fa9c6d9cce71d3cf97c291b2167e3 F ext/wasm/testing2.js 88f40ef3cd8201bdadd120a711c36bbf0ce56cc0eab1d5e7debb71fed7822494 F ext/wasm/wasmfs.make 3cce1820006196de140f90f2da4b4ea657083fb5bfee7d125be43f7a85748c8f @@ -2029,8 +2029,8 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0 -P 64ebcbe41615a6d7776597564105ea7638e4a9095a764ea558c2620640429cf8 -R 624d71ac921985ea3965d7f2eda6af5a +P 29db7de79232c21d19b91bb0fc253114e02e21dd9bf90619453dbe72a4c8bf7f +R bd74fd06e0648815273bfbe7fab3eb12 U stephan -Z 9261b853936f36a4eb842018d682a85c +Z 599287064c24d1dab9b272142121f171 # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index be02a92425..d85382de39 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -29db7de79232c21d19b91bb0fc253114e02e21dd9bf90619453dbe72a4c8bf7f \ No newline at end of file +1fa019c88deac6b6e5155b620bdab1d7145846290daafb9adbefcf4f0fe542cf \ No newline at end of file