ceifa / wasmoon

A real lua 5.4 VM with JS bindings made with webassembly
MIT License
462 stars 27 forks source link

Misbehaving negative integers #54

Closed plsmphnx closed 8 months ago

plsmphnx commented 2 years ago

Negative integers do not appear to translate correctly from JS, resulting instead in a large positive integer within the Lua runtime (specifically, 2^32 minus the value, which makes it look like a signed/unsigned issue). This can be observed via the following code:

lua.global.set('value', -1)
await lua.doString('print(value)')
// 4294967295

The same result occurs with function return values, etc.

ceifa commented 2 years ago

Seems like a problem with the lua_pushinteger function, the weird thing is that the implementation on wasmoon side it's pretty simple.

tims-bsquare commented 2 years ago

I did some digging into this and it's because by default Lua expects to be compiled for 64-bit whereas Emscripten/WASM only support 32 bit. In luaconf.h change #define LUA_32BITS 0 to #define LUA_32BITS 1

This will be a bit tricky because it doesn't keep a local copy of Lua. You could use the Linux patch command

ceifa commented 2 years ago

Nice found. I tried to pass -DLUA_32BITS=1 to emcc but doesn't work, even saying the macro was redefined...

plsmphnx commented 2 years ago

It looks like setting that option via compiler flags was removed a little while back: https://github.com/lua/lua/commit/dee6433a89b088a1f8da9531a92a2a2693e5dac7

Barring maintaining a copy of luaconf.h, a patch (either via patch or a simple JS script) is likely the best bet.

clecompt-msft commented 2 years ago

Another one-line patch option would be sed -i "s/^#define LUA_32BITS\t0$/#define LUA_32BITS\t1/" luaconf.h. I'm not sure the best way to make use of this outside of leaving an unstaged change in the submodule or making a copy before building, though, both of which feel unwieldly. A quick test with this does appear to resolve the issue, though the "limit memory" test stops throwing the expected error.

tims-bsquare commented 2 years ago

Yeah, with the limit memory test the Lua heap has enough space available to run the test with 32 bit ints, bumping the loop count up to 50 or so makes that work

ceifa commented 2 years ago

Fixed using the @clecompt-msft suggestion, I think it works for now.

pnemere commented 1 year ago

Hi! I've just started experimenting with Wasmoon for our project and I'm actually having an issue now with integers overflowing within Lua. I tracked it back to this #define LUA_32BITS 1 line in the build file.

I just wanted to ask, is it possible something was overlooked here? From my readings about WASM it appears that it should support 32 and 64bit integers (and 32+64 for floats too). Is it possible switching to build a 32bit Lua has just masked a different issue where the -1 integer was treated incorrectly along the way during the "set" call?

Given Lua supports 64bit integers, and so does WASM (https://webassembly.org/roadmap/) it seems a shame to exclude the possibility completely!

pnemere commented 1 year ago

FYI I have compiled this on my machine with #define LUA_32BITS 0 and all my code works fine. I don't ever call lua.global.set('value', -1) directly though, I return JS arrays (which I made sure map nicely to tables with the marshalling code in this lib!) - these are returned from JS functions that my Lua code calls.

TomWoodward commented 1 year ago

i also ran into the integers overflowing when trying to pass ms epochs between js and lua, easily reproducible:

> new (require("wasmoon").LuaFactory)().createEngine().then(lua => {
  const time = Date.now();
  lua.global.set('time', time);
  console.log(time);
  lua.doString("print(time)")
})
> 1689031554550
1109407222
itz-coffee commented 9 months ago

i also ran into the integers overflowing when trying to pass ms epochs between js and lua, easily reproducible:

> new (require("wasmoon").LuaFactory)().createEngine().then(lua => {
  const time = Date.now();
  lua.global.set('time', time);
  console.log(time);
  lua.doString("print(time)")
})
> 1689031554550
1109407222

I'm running into this error as well