From 62720056b1e2c2bac4dcb8fea53c692ccb838420 Mon Sep 17 00:00:00 2001 From: Tim Stableford Date: Mon, 20 Nov 2023 11:07:13 +0000 Subject: [PATCH 1/4] Fix #54 misbehaving integers * Modify build.sh to use 64 bit floats but 32 bit ints wasm supports this but not 64 bit ints * When pushing a number see if it's within 32 bits as well as being an int if it's not then push it as a 64 bit float * Add tests for negative numbers and numbers greater than 32 bit * Fixed build issue with current emscripten https://github.com/emscripten-core/emscripten/issues/20721 * Updated docs with edge cases to hopefully close off #22 #23 and #54 --- README.md | 80 +++++++++++++++++++++++++++++++++++++++++++++ build.sh | 13 +++++--- src/thread.ts | 7 +++- test/engine.test.js | 51 +++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 9b0a7f6..2249b0c 100644 --- a/README.md +++ b/README.md @@ -181,3 +181,83 @@ npm run build:wasm:dev # build lua npm run build # build the js code/bridge npm test # ensure everything it's working fine ``` + +## Edge Cases + +### Numbers +WASM does not support 64 bit integers only 32 bit integers and 64 bit doubles. If a number is an integer and will fit within a Lua integer then it will be pushed as a Lua native integer type. However, if it exceeds that size even if it is still an integer it will be pushed as a 64 bit double. This is unlikely to effect inteoperability with JS since JS treats all numbers the same but should allow some optimisation within Lua for smaller numbers. + +### Null + +`null` is not exposed to Lua and it has no awareness of it which can cause some issues when using it a table. `nil` is equivalent to `undefined`. Issue #39 tracks this and a workaround until `null` is added into Wasmoon. + +### Promises + +Promises can be await'd from Lua with some caveats detailed in the below section. To await a Promise call `:await()` on it which will yield the Lua execution until the promise completes. + +```js +const { LuaFactory } = require('wasmoon') +const factory = new LuaFactory() +const lua = await factory.createEngine() + +try { + lua.global.set('sleep', (length) => new Promise(resolve => setTimeout(resolve, length))) + await lua.doString(` + sleep(1000):await() + `) +} finally { + lua.global.close() +} +``` + +### Async/Await + +It's not possible to await in a callback from JS into Lua. This is a limitation of Lua but there are some workarounds. It can also be encountered when yielding at the top-level of a file. An example where you might encounter this is a snippet like this: +```js +local res = sleep(1):next(function () + sleep(10):await() + return 15 +end) +print("res", res:await()) +``` +Which will throw an error like this: +``` +Error: Lua Error(ErrorRun/2): cannot resume dead coroutine + at Thread.assertOk (/home/tstableford/projects/wasmoon/dist/index.js:409:23) + at Thread. (/home/tstableford/projects/wasmoon/dist/index.js:142:22) + at Generator.throw () + at rejected (/home/tstableford/projects/wasmoon/dist/index.js:26:69) +``` +Or like this: +``` +attempt to yield across a C-call boundary +``` + +You can workaround this by doing something like below: +```lua +function async(callback) + return function(...) + local co = coroutine.create(callback) + local safe, result = coroutine.resume(co, ...) + + return Promise.create(function(resolve, reject) + local function step() + if coroutine.status(co) == "dead" then + local send = safe and resolve or reject + return send(result) + end + + safe, result = coroutine.resume(co) + + if safe and result == Promise.resolve(result) then + result:finally(step) + else + step() + end + end + + result:finally(step) + end) + end +end +``` \ No newline at end of file diff --git a/build.sh b/build.sh index d175eb1..4c353d8 100755 --- a/build.sh +++ b/build.sh @@ -7,15 +7,18 @@ LUA_SRC=$(ls ./lua/*.c | grep -v "luac.c" | grep -v "lua.c" | tr "\n" " ") extension="" if [ "$1" == "dev" ]; then - extension="$extension -O0 -g3 -s ASSERTIONS=1 -s SAFE_HEAP=1 -s STACK_OVERFLOW_CHECK=2" + extension="-O0 -g3 -s ASSERTIONS=1 -s SAFE_HEAP=1 -s STACK_OVERFLOW_CHECK=2" else - extension="$extension -O3 --closure 1" + # ASSERTIONS=1 required with optimisations and strict mode. https://github.com/emscripten-core/emscripten/issues/20721 + extension="-O3 --closure 1 -s ASSERTIONS=1" fi +# Instead of telling Lua to be 32 bit for both floats and ints override the default +# int type to 32 bit and leave the float as 64 bits. if [[ "$OSTYPE" == "darwin"* ]]; then - sed -i '' "s/^#define LUA_32BITS\t0$/#define LUA_32BITS\t1/" ./lua/luaconf.h + sed -E -i '' "s/#define LUA_INT_DEFAULT\s+LUA_INT_LONGLONG/#define LUA_INT_DEFAULT \tLUA_INT_INT/" ./lua/luaconf.h else - sed -i "s/^#define LUA_32BITS\t0$/#define LUA_32BITS\t1/" ./lua/luaconf.h + sed -E -i "s/#define LUA_INT_DEFAULT\s+LUA_INT_LONGLONG/#define LUA_INT_DEFAULT \tLUA_INT_INT/" ./lua/luaconf.h fi emcc \ @@ -44,7 +47,7 @@ emcc \ -s STRICT=1 \ -s EXPORT_ES6=1 \ -s NODEJS_CATCH_EXIT=0 \ - -s NODEJS_CATCH_REJECTION=0 \ + -s NODEJS_CATCH_REJECTION=0 \ -s MALLOC=emmalloc \ -s STACK_SIZE=1MB \ -s EXPORTED_FUNCTIONS="[ diff --git a/src/thread.ts b/src/thread.ts index 29f2381..bd14898 100755 --- a/src/thread.ts +++ b/src/thread.ts @@ -23,6 +23,9 @@ export interface OrderedExtension { // When the debug count hook is set, call it every X instructions. const INSTRUCTION_HOOK_COUNT = 1000 +// These reflect math.maxinteger and math.mininteger with 32 bit ints in Lua. +const MAX_SAFE_INTEGER = Math.pow(2, 31) - 1 +const MIN_SAFE_INTEGER = -Math.pow(2, 31) export default class Thread { public readonly address: LuaState @@ -204,7 +207,9 @@ export default class Thread { this.lua.lua_pushnil(this.address) break case 'number': - if (Number.isInteger(target)) { + // Only push it as an integer if it fits within 32 bits, otherwise treat it as a double. + // This is because wasm doesn't support 32 bit ints but does support 64 bit floats. + if (Number.isInteger(target) && target <= MAX_SAFE_INTEGER && target >= MIN_SAFE_INTEGER) { this.lua.lua_pushinteger(this.address, target) } else { this.lua.lua_pushnumber(this.address, target) diff --git a/test/engine.test.js b/test/engine.test.js index 084732b..0b63982 100755 --- a/test/engine.test.js +++ b/test/engine.test.js @@ -660,4 +660,55 @@ describe('Engine', () => { expect(res).to.be.equal(str) }) + + it('negative integers should be pushed and retrieved as string', async () => { + const engine = await getEngine() + engine.global.set('value', -1) + + const res = await engine.doString(`return tostring(value)`) + + expect(res).to.be.equal('-1') + }) + + it('negative integers should be pushed and retrieved as number', async () => { + const engine = await getEngine() + engine.global.set('value', -1) + + const res = await engine.doString(`return value`) + + expect(res).to.be.equal(-1) + }) + + it('number greater than 32 bit int should be pushed and retrieved as string', async () => { + const engine = await getEngine() + const value = 1689031554550 + engine.global.set('value', value) + + const res = await engine.doString(`return tostring(value)`) + + // Since it's a float in Lua it will be prefixed with .0 from tostring() + expect(res).to.be.equal(`${String(value)}.0`) + }) + + it('number greater than 32 bit int should be pushed and retrieved as number', async () => { + const engine = await getEngine() + const value = 1689031554550 + engine.global.set('value', value) + + const res = await engine.doString(`return value`) + + expect(res).to.be.equal(value) + }) + + it('print max integer as 32 bits', async () => { + const engine = await getEngine() + const res = await engine.doString(`return math.maxinteger`) + expect(res).to.be.equal(2147483647) + }) + + it('print min integer as 32 bits', async () => { + const engine = await getEngine() + const res = await engine.doString(`return math.mininteger`) + expect(res).to.be.equal(-2147483648) + }) }) From 5534d35ad97f76d3d686ae01cb8fe19bd46ef76d Mon Sep 17 00:00:00 2001 From: Tim Stableford Date: Thu, 30 Nov 2023 09:58:01 +0000 Subject: [PATCH 2/4] Fix callByteCode can segfault when returning a Lua referenced value --- src/engine.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/engine.ts b/src/engine.ts index 0660bf0..a3f2511 100755 --- a/src/engine.ts +++ b/src/engine.ts @@ -70,9 +70,15 @@ export default class LuaEngine { loader(thread) const result = await thread.run(0) if (result.length > 0) { + // Move all stack results to the global state to avoid referencing the thread values + // which will be cleaned up in the finally below. this.cmodule.lua_xmove(thread.address, this.global.address, result.length) + // The shenanigans here are to return the first reuslt value on the stack. + // Say there's 2 values at stack indexes 1 and 2. Then top is 2, result.length is 2. + // That's why there's a + 1 sitting at the end. + return this.global.getValue(this.global.getTop() - result.length + 1) } - return result[0] + return undefined } finally { // Pop the read on success or failure this.global.remove(threadIndex) From 6bcda769f293c4235080015292c65972c94bf54d Mon Sep 17 00:00:00 2001 From: Tim Stableford Date: Thu, 30 Nov 2023 10:10:56 +0000 Subject: [PATCH 3/4] Run linter on README --- README.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 2249b0c..3434bf3 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,7 @@ npm test # ensure everything it's working fine ## Edge Cases ### Numbers + WASM does not support 64 bit integers only 32 bit integers and 64 bit doubles. If a number is an integer and will fit within a Lua integer then it will be pushed as a Lua native integer type. However, if it exceeds that size even if it is still an integer it will be pushed as a 64 bit double. This is unlikely to effect inteoperability with JS since JS treats all numbers the same but should allow some optimisation within Lua for smaller numbers. ### Null @@ -201,7 +202,7 @@ const factory = new LuaFactory() const lua = await factory.createEngine() try { - lua.global.set('sleep', (length) => new Promise(resolve => setTimeout(resolve, length))) + lua.global.set('sleep', (length) => new Promise((resolve) => setTimeout(resolve, length))) await lua.doString(` sleep(1000):await() `) @@ -213,6 +214,7 @@ try { ### Async/Await It's not possible to await in a callback from JS into Lua. This is a limitation of Lua but there are some workarounds. It can also be encountered when yielding at the top-level of a file. An example where you might encounter this is a snippet like this: + ```js local res = sleep(1):next(function () sleep(10):await() @@ -220,7 +222,9 @@ local res = sleep(1):next(function () end) print("res", res:await()) ``` + Which will throw an error like this: + ``` Error: Lua Error(ErrorRun/2): cannot resume dead coroutine at Thread.assertOk (/home/tstableford/projects/wasmoon/dist/index.js:409:23) @@ -228,12 +232,15 @@ Error: Lua Error(ErrorRun/2): cannot resume dead coroutine at Generator.throw () at rejected (/home/tstableford/projects/wasmoon/dist/index.js:26:69) ``` + Or like this: + ``` attempt to yield across a C-call boundary ``` You can workaround this by doing something like below: + ```lua function async(callback) return function(...) @@ -260,4 +267,4 @@ function async(callback) end) end end -``` \ No newline at end of file +``` From baebf6629fdde4aaef38d22a9525381e00e33f34 Mon Sep 17 00:00:00 2001 From: Tim Stableford Date: Thu, 30 Nov 2023 14:43:46 +0000 Subject: [PATCH 4/4] Added TODO for disabling assertions in prod build --- build.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/build.sh b/build.sh index 4c353d8..c033f64 100755 --- a/build.sh +++ b/build.sh @@ -9,6 +9,7 @@ if [ "$1" == "dev" ]; then extension="-O0 -g3 -s ASSERTIONS=1 -s SAFE_HEAP=1 -s STACK_OVERFLOW_CHECK=2" else + # TODO: This appears to be a bug in emscripten. Disable assertions when that bug is resolved or a workaround found. # ASSERTIONS=1 required with optimisations and strict mode. https://github.com/emscripten-core/emscripten/issues/20721 extension="-O3 --closure 1 -s ASSERTIONS=1" fi