Skip to content

Misbehaving negative integers #54

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

Closed
plsmphnx opened this issue Jul 4, 2022 · 11 comments
Closed

Misbehaving negative integers #54

plsmphnx opened this issue Jul 4, 2022 · 11 comments

Comments

@plsmphnx
Copy link

plsmphnx commented Jul 4, 2022

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
Copy link
Owner

ceifa commented Jul 5, 2022

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
Copy link
Contributor

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
Copy link
Owner

ceifa commented Jul 14, 2022

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

@plsmphnx
Copy link
Author

It looks like setting that option via compiler flags was removed a little while back: lua/lua@dee6433

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

@clecompt-msft
Copy link

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
Copy link
Contributor

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
Copy link
Owner

ceifa commented Jul 15, 2022

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

@ceifa ceifa closed this as completed Jul 15, 2022
@pnemere
Copy link

pnemere commented Feb 7, 2023

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!

@ceifa ceifa reopened this Feb 7, 2023
@pnemere
Copy link

pnemere commented Feb 10, 2023

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
Copy link

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
Copy link

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

tims-bsquare added a commit to tims-bsquare/wasmoon that referenced this issue Nov 20, 2023
* 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
@ceifa ceifa closed this as completed in 6272005 Nov 30, 2023
ceifa added a commit that referenced this issue Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants