Skip to content
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

Fix #54 misbehaving integers #97

Merged
merged 4 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,3 +181,90 @@ 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.<anonymous> (/home/tstableford/projects/wasmoon/dist/index.js:142:22)
at Generator.throw (<anonymous>)
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
```
13 changes: 8 additions & 5 deletions build.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing this, I made the following update to the if/else block at the bottom to avoid leaving local changes to the lua submodule.:

if [[ "$OSTYPE" == "darwin"* ]]; then
    sed -E -i '' "s/#define LUA_INT_DEFAULT\s+LUA_INT_INT/#define LUA_INT_DEFAULT\t\tLUA_INT_LONGLONG/" ./lua/luaconf.h
else
    sed -E -i "s/#define LUA_INT_DEFAULT\s+LUA_INT_INT/#define LUA_INT_DEFAULT\t\tLUA_INT_LONGLONG/" ./lua/luaconf.h
fi

Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is a bug on emscripten? Assertions are probably unnecessary and just increase the code size and slow down a little bit. Should have a TODO to remove it later, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell it's a bug in emscripten, the one linked. I'd be quite happy if you found time to take a second look though just in case. I'll add that TODO in

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 \
Expand Down Expand Up @@ -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="[
Expand Down
8 changes: 7 additions & 1 deletion src/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion src/thread.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
51 changes: 51 additions & 0 deletions test/engine.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
})